-
Notifications
You must be signed in to change notification settings - Fork 48.8k
[compiler] Override type provider for hook-like names #30868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation. [ghstack-poisoned]
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation. [ghstack-poisoned]
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation. [ghstack-poisoned]
) { | ||
return propertyType; | ||
} | ||
return this.#getCustomHookType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might produce invalid compilation output on false positives (i.e. functions that are hook-named but not hooks) as hook calls freeze args and returned results whereas regular function calls don't. Imports being renamed nonhook <--> hook look a bit suspicious, but we haven't had any practical issues.
import {foo as useHookNameForNonHook} ...
Seems reasonable as our internal use is currently pretty limited. It might be a good idea to warn / bailout here in the future if we find cases of calls being incorrectly inferred as hooks.
@@ -48,6 +48,14 @@ export function makeSharedRuntimeTypeProvider({ | |||
returnType: {kind: 'type', name: 'Primitive'}, | |||
returnValueKind: ValueKindEnum.Primitive, | |||
}, | |||
useArrayConcatNotTypedAsHook: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit like a InvalidConfigError
(and something we can validate when parsing out EnvironmentConfigs).
It's one thing to have assume that unknown properties are hooks based on naming, but, if the type provider lists a hook-named property as a function, I think we should either respect that or throw with an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall makes sense! A few non-blocking suggestions / questions but feel free to land without any changes
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation. [ghstack-poisoned]
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation. [ghstack-poisoned]
Yeah, good feedback. I was torn on whether to go the validation route or this approach, i'm gonna switch to just validating that the config was correct. |
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). [ghstack-poisoned]
Stack from ghstack (oldest at bottom):
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.