fix(types): add ProviderDefaults to fix provider type resolution#2056
fix(types): add ProviderDefaults to fix provider type resolution#2056kaki0704 wants to merge 4 commits into
Conversation
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2056 +/- ##
==========================================
+ Coverage 6.99% 32.52% +25.52%
==========================================
Files 78 7 -71
Lines 3629 372 -3257
Branches 140 131 -9
==========================================
- Hits 254 121 -133
+ Misses 3326 194 -3132
- Partials 49 57 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // @ts-expect-error this is not a valid modifier for ipx | ||
| alkj: false, |
There was a problem hiding this comment.
this is a correct test to make sure that modifiers are typed correctly (in other words, you can't pass a nonsense modifier to them)
we wouldn't want to get rid of this test
There was a problem hiding this comment.
I've reverted the @ts-expect-error removal. I now understand this is a valid test to ensure invalid modifiers are caught by the type system.
| interface ProviderDefaults { | ||
| provider: 'ipx' | ||
| } |
There was a problem hiding this comment.
I don't believe this is a correct fix - this file is only used when developing nuxt/image and is not included in the dist files... it won't make any difference to the experience of an end user
There was a problem hiding this comment.
You're correct that this file isn't included in the dist, so it wouldn't help end users.
danielroe
left a comment
There was a problem hiding this comment.
I don't think this PR would have any effect on the built code of the module.
if you think it does, would you build and diff it with https://app.unpkg.com/@nuxt/image@2.0.0/files/dist/module.d.ts?
🙏
|
Thank you for taking the time to review and for the helpful feedback! 🙏 |
| } | ||
|
|
||
| type DefaultProvider = ProviderDefaults extends Record<'provider', unknown> ? ProviderDefaults['provider'] : never | ||
| type DefaultProvider = ProviderDefaults extends Record<'provider', infer P> ? P : keyof ConfiguredImageProviders |
There was a problem hiding this comment.
I still don't think this is the right fix - we need to know the specific default provider (likely ipx) in case provider is not passed into any image functions.
|
I've taken another look. Not entirely sure I got it right, but I added Could you please check if this is the correct approach?🙇🏻♂️ |
|
I don't think this is right either. that interface is populated dynamically via types that nuxt generates, which should certainly include ipx |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request modifies Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
resolves #2014
🔗 Linked issue
resolves #2014
❓ Type of change
📚 Description
The
ProviderDefaultsinterface was missing fromsrc/index.d.ts, causingDefaultProvidertype to resolve toneverduring development.This happened because in
src/types/image.ts, theDefaultProvidertype is defined as:Without
ProviderDefaults.providerbeing defined, the conditional type resolves tonever, breaking provider type inference.Changes:
ProviderDefaultsinterface withprovider: 'ipx'tosrc/index.d.ts@ts-expect-errordirectives from tests (now that types work correctly)Testing:
pnpm test:typespasses