Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.

fix(nuxt): warn to provide a key for useFetch with transform option - #4590

Merged
pi0 merged 5 commits into
mainfrom
fix/key-transform
Apr 27, 2022
Merged

fix(nuxt): warn to provide a key for useFetch with transform option#4590
pi0 merged 5 commits into
mainfrom
fix/key-transform

Conversation

@danielroe

Copy link
Copy Markdown
Member

🔗 Linked issue

resolves nuxt/nuxt#13790

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR addresses two issues:

  1. In production the transform function .toString() differs between server/client so the generated key (from ohash) doesn't match.
  2. When there is a mismatch (no key in payload), then currently we just skip refreshing, when probably we want to re-fetch on client-side.

Other options:

  1. generate a key at build-time from hash of transform function
  2. exclude transform function from hash generation

It feels like the safest option is to require a user to provide a custom key in the event of using transform.

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added bug Something isn't working 🔨 p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Apr 25, 2022
@danielroe
danielroe requested a review from pi0 April 25, 2022 12:51
@danielroe danielroe self-assigned this Apr 25, 2022
@netlify

netlify Bot commented Apr 25, 2022

Copy link
Copy Markdown

Deploy Preview for nuxt3-docs ready!

Name Link
🔨 Latest commit cdd83a5
🔍 Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/626929bb42e6c700091a27ca
😎 Deploy Preview https://deploy-preview-4590--nuxt3-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pi0

pi0 commented Apr 25, 2022

Copy link
Copy Markdown
Member

I think we can exclude options that are not related to fetch like transform from hash transformation. hashing it to distinguish by request.

Comment thread packages/nuxt/src/app/composables/fetch.ts Outdated
Co-authored-by: Sébastien Chopin <seb@nuxtjs.com>
@danielroe

Copy link
Copy Markdown
Member Author

I think we can exclude options that are not related to fetch like transform from hash transformation. hashing it to distinguish by request.

I would say that transform should make a difference to the hash. Consider:

await useFetch('/api/test', { transform: data => data[0] })
await useFetch('/api/test')

These two calls will result in different data, and if we don't enforce a key generation then there's a danger of mismatching payloads.

@pi0

pi0 commented Apr 26, 2022

Copy link
Copy Markdown
Member

Okay i'm fine with adding warning until find a better way for auto hashing. Can we in the meantime remove transform from hash when making warning?

@pi0 pi0 changed the title fix(nuxt): require users to provide a key for useFetch + transform fix(nuxt): require users to provide a key for useFetch with transform option Apr 27, 2022
@pi0 pi0 changed the title fix(nuxt): require users to provide a key for useFetch with transform option fix(nuxt): warn to provide a key for useFetch with transform option Apr 27, 2022
@pi0
pi0 merged commit 7e912e7 into main Apr 27, 2022
@pi0
pi0 deleted the fix/key-transform branch April 27, 2022 16:19
@pi0 pi0 mentioned this pull request May 2, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3.x bug Something isn't working 🔨 p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When build app useFetch with transform option will return data with value null on client side

3 participants