fix(nuxt): correct type inference when using transform with getCachedData#34800
fix(nuxt): correct type inference when using transform with getCachedData#34800noriyuki-shimizu wants to merge 2 commits intonuxt:mainfrom
Conversation
|
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
Merging this PR will not alter performance
Comparing Footnotes
|
|
@danielroe |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request changes type declarations for 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thank you ❤️ |
|
@danielroe |
|
The regression test uses |
|
@noriyuki-shimizu checking, the regression test actually seems to pass on the main branch - would you investigate please? 🙏 |
|
@danielroe @afurm I investigated thoroughly and updated the regression test with the following improvements:
Important note: during investigation, I confirmed that the exact reproduction from issue #29567 (explicit That said, I should be transparent: this test also passes on |
|
@danielroe |
|
We've flagged this as a potential contribution without a human behind it. We welcome the thoughtful use of AI tools when contributing to Nuxt, but ask all contributors to follow two core principles:
Please review our AI-assisted contribution guidelines and update this contribution if needed. If this was flagged in error, we apologise! 😳 Just let us know. 🙏 |
|
as far as I understand it, the test you've added passes on both main and this branch - and this PR doesn't solve the linked issue. is that right? |
|
@danielroe
The changes in this PR (removing the custom I'm happy to either close this PR or rework it as a simple cleanup (removing the issue link), whichever you prefer. |
|
are you a bot or a human? |
|
I'm a human 🙂 |
|
okay, that’s all good! just wanted to check there was a ‘responsible adult’ in the room 😆 i am not sure there is a reason to merge this PR if we can’t reproduce a bug that it fixes maybe worth some more time thinking about it 🙏 |
|
danielroe raises a valid point — if the test passes on both branches, the PR is currently a cleanup without a regression test for the reported bug. The type change itself (moving |
|
... @afurm are you a human? |
|
Yes, human 🙂 I occasionally use AI assistance to draft comments faster, but all technical opinions are mine and I verify things locally before posting. |
🔗 Linked issue
Fixes #29567
📚 Description
Problem
When using
useAsyncDatawith bothtransformandgetCachedData, TypeScript incorrectly infersDataTfromgetCachedData's return type instead of from thetransformfunction.Solution
Two changes in asyncData.ts:
Remove custom
NoInfertype — The custom implementation[T][T extends any ? 0 : never]did not fully prevent inference leakage whenundefinedwas part of the union. TypeScript 5.4+ provides a built-inNoInferutility type that handles this correctly.Move
undefinedinsideNoInfer— ChangedgetCachedDatareturn type fromNoInfer<DataT> | undefinedtoNoInfer<DataT | undefined>. This ensures TypeScript does not usegetCachedData's return type as an inference site forDataT.A regression test was added to verify
DataTis correctly inferred fromtransformwhengetCachedDatais also provided.Notes
NoInferimplementation, which caused type tests to fail. This PR resolves the root cause by removing the customNoInferin favor of the built-in one.NoInferutility type (available since TypeScript 5.4).