-
Notifications
You must be signed in to change notification settings - Fork 49.4k
Change autodeps configuration #33800
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
@@ -183,6 +183,16 @@ export function inferEffectDependencies(fn: HIRFunction): void { | |||
autodepFnLoads.has(callee.identifier.id) && | |||
value.args[0].kind === 'Identifier' | |||
) { | |||
const autodepsArgExpectedIndex = autodepFnLoads.get( |
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.
Nit: should we also also remove the check on value.args.length > 1 && autodepsArgIndex > 0
above, since we also assert this in the environment? This way we'd get InvalidReact
errors for useEffect(AUTODEPS)
if that's something we care about
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.
I think value.args.length > 0 should suffice. need to keep > 0 to check the 0th index in the RHS of && and autodepsArgIndex => 0 because we should only be trying to insert the deps if AUTODEPS exists in the arg list. That should catch the useEffect(AUTODEPS)
case, will add a test
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.
I ended up going for a different approach here because if we throw an error in the autodeps pass then we don't report the build error (? I should follow up with a fix for that).
In ValidateNoUntransformedReferences we instead check to see if any of the args are AUTODEPS instead of just the expected one. This surfaced a handful of new files that were not updated to the AUTODEPS approach, see the updated tests!
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.
Yeah this makes a lot of sense, lets ship it!
This surfaced a handful of new files that were not updated to the AUTODEPS approach
Ohh I'm curious what files were -- were we previously supplying AUTODEPS
in an unexpected position, e.g. autodepsIndex + 1
?
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.
Makes sense!
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * #33800 * __->__ #33799
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * #33800 * __->__ #33799 DiffTrain build for [dffacc7](dffacc7)
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * #33800 * __->__ #33799 DiffTrain build for [dffacc7](dffacc7)
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * facebook#33800 * __->__ facebook#33799 DiffTrain build for [dffacc7](facebook@dffacc7)
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * facebook#33800 * __->__ facebook#33799 DiffTrain build for [dffacc7](facebook@dffacc7)
No description provided.