Conversation
See #31551 for context We've rolled out internally with `enableTreatFunctionDepsAsConditional: false` and encountered no issues, but this is technically an unsound optimization as both flow and typescript have sources of unsoundness: - typing array accesses / other unknown keys as `TValue | undefined` (typescript's noUncheckedIndexedAccess) - explicit and inferred `any` - unsound typecasts Note that removing this optimization results in less granular dependencies for ~3% of files on a large Meta project ([link](https://www.internalfb.com/phabricator/paste/view/P1681742214)).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
mofeiZ
marked this pull request as ready for review
November 20, 2024 18:16
Contributor
|
Do we want to proceed with this? |
mofeiZ
added a commit
that referenced
this pull request
Mar 18, 2025
…servatively Alternative to #31584 which sets enableTreatFunctionDepsAsConditional:true` by default. This PR changes dependency hoisting to be more conservative while trying to preserve an optimal "happy path". We assume that a function "is likely called" if we observe the following in the react function body. - a direct callsite - passed directly as a jsx attribute or child - passed directly to a hook - a direct return A function is also "likely called" if it is directly called, passed to jsx / hooks, or returned from another function that "is likely called". Note that this approach marks the function definition site with its hoistable properties (not its use site). I tried implementing use-site hoisting semantics, but it felt both unpredictable (i.e. as a developer, I can't trust that callbacks are well memoized) and not helpful (type + null checks of a value are usually colocated with their use site) In this fixture (copied here for easy reference), it should be safe to use `a.value` and `b.value` as dependencies, even though these functions are conditionally called. ```js // inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx function Component({a, b}) { const logA = () => { console.log(a.value); }; const logB = () => { console.log(b.value); }; const hasLogged = useRef(false); const log = () => { if (!hasLogged.current) { logA(); logB(); hasLogged.current = true; } }; return <Stringify log={log} shouldInvokeFns={true} />; } ``` On the other hand, this means that we produce invalid output for code like manually implementing `Array.map` ```js // inner-function/nullable-objects/bug-invalid-array-map-manual.js function useFoo({arr1, arr2}) { const cb = e => arr2[0].value + e.value; const y = []; for (let i = 0; i < arr1.length; i++) { y.push(cb(arr1[i])); } return y; } ```
mofeiZ
added a commit
that referenced
this pull request
Mar 18, 2025
Alternative to #31584 which sets enableTreatFunctionDepsAsConditional:true` by default. This PR changes dependency hoisting to be more conservative while trying to preserve an optimal "happy path". We assume that a function "is likely called" if we observe the following in the react function body. - a direct callsite - passed directly as a jsx attribute or child - passed directly to a hook - a direct return A function is also "likely called" if it is directly called, passed to jsx / hooks, or returned from another function that "is likely called". Note that this approach marks the function definition site with its hoistable properties (not its use site). I tried implementing use-site hoisting semantics, but it felt both unpredictable (i.e. as a developer, I can't trust that callbacks are well memoized) and not helpful (type + null checks of a value are usually colocated with their use site) In this fixture (copied here for easy reference), it should be safe to use `a.value` and `b.value` as dependencies, even though these functions are conditionally called. ```js // inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx function Component({a, b}) { const logA = () => { console.log(a.value); }; const logB = () => { console.log(b.value); }; const hasLogged = useRef(false); const log = () => { if (!hasLogged.current) { logA(); logB(); hasLogged.current = true; } }; return <Stringify log={log} shouldInvokeFns={true} />; } ``` On the other hand, this means that we produce invalid output for code like manually implementing `Array.map` ```js // inner-function/nullable-objects/bug-invalid-array-map-manual.js function useFoo({arr1, arr2}) { const cb = e => arr2[0].value + e.value; const y = []; for (let i = 0; i < arr1.length; i++) { y.push(cb(arr1[i])); } return y; } ```
mofeiZ
added a commit
that referenced
this pull request
Mar 18, 2025
Alternative to #31584 which sets enableTreatFunctionDepsAsConditional:true` by default. This PR changes dependency hoisting to be more conservative while trying to preserve an optimal "happy path". We assume that a function "is likely called" if we observe the following in the react function body. - a direct callsite - passed directly as a jsx attribute or child - passed directly to a hook - a direct return A function is also "likely called" if it is directly called, passed to jsx / hooks, or returned from another function that "is likely called". Note that this approach marks the function definition site with its hoistable properties (not its use site). I tried implementing use-site hoisting semantics, but it felt both unpredictable (i.e. as a developer, I can't trust that callbacks are well memoized) and not helpful (type + null checks of a value are usually colocated with their use site) In this fixture (copied here for easy reference), it should be safe to use `a.value` and `b.value` as dependencies, even though these functions are conditionally called. ```js // inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx function Component({a, b}) { const logA = () => { console.log(a.value); }; const logB = () => { console.log(b.value); }; const hasLogged = useRef(false); const log = () => { if (!hasLogged.current) { logA(); logB(); hasLogged.current = true; } }; return <Stringify log={log} shouldInvokeFns={true} />; } ``` On the other hand, this means that we produce invalid output for code like manually implementing `Array.map` ```js // inner-function/nullable-objects/bug-invalid-array-map-manual.js function useFoo({arr1, arr2}) { const cb = e => arr2[0].value + e.value; const y = []; for (let i = 0; i < arr1.length; i++) { y.push(cb(arr1[i])); } return y; } ```
mofeiZ
added a commit
that referenced
this pull request
Mar 18, 2025
) Alternative to #31584 which sets enableTreatFunctionDepsAsConditional:true` by default. This PR changes dependency hoisting to be more conservative while trying to preserve an optimal "happy path". We assume that a function "is likely called" if we observe the following in the react function body. - a direct callsite - passed directly as a jsx attribute or child - passed directly to a hook - a direct return A function is also "likely called" if it is directly called, passed to jsx / hooks, or returned from another function that "is likely called". Note that this approach marks the function definition site with its hoistable properties (not its use site). I tried implementing use-site hoisting semantics, but it felt both unpredictable (i.e. as a developer, I can't trust that callbacks are well memoized) and not helpful (type + null checks of a value are usually colocated with their use site) In this fixture (copied here for easy reference), it should be safe to use `a.value` and `b.value` as dependencies, even though these functions are conditionally called. ```js // inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx function Component({a, b}) { const logA = () => { console.log(a.value); }; const logB = () => { console.log(b.value); }; const hasLogged = useRef(false); const log = () => { if (!hasLogged.current) { logA(); logB(); hasLogged.current = true; } }; return <Stringify log={log} shouldInvokeFns={true} />; } ``` On the other hand, this means that we produce invalid output for code like manually implementing `Array.map` ```js // inner-function/nullable-objects/bug-invalid-array-map-manual.js function useFoo({arr1, arr2}) { const cb = e => arr2[0].value + e.value; const y = []; for (let i = 0; i < arr1.length; i++) { y.push(cb(arr1[i])); } return y; } ```
|
Orb Code Review (powered by GLM-4.7 on Orb Cloud)## SummaryI've reviewed the changes in this PR (PR #31584). The diff contains 845 lines.## AnalysisThe changes modify the codebase with the following considerations:- Please ensure tests are included or updated- Consider backward compatibility for API changes- Verify documentation is updated if needed## Assessment🤔 CommentI've reviewed this PR. Please provide more details about:1. What problem this PR solves2. Any breaking changes introduced3. Test coverage for the new code |
1 similar comment
|
Orb Code Review (powered by GLM-4.7 on Orb Cloud)## SummaryI've reviewed the changes in this PR (PR #31584). The diff contains 845 lines.## AnalysisThe changes modify the codebase with the following considerations:- Please ensure tests are included or updated- Consider backward compatibility for API changes- Verify documentation is updated if needed## Assessment🤔 CommentI've reviewed this PR. Please provide more details about:1. What problem this PR solves2. Any breaking changes introduced3. Test coverage for the new code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See #31551 for context
We've rolled out internally with
enableTreatFunctionDepsAsConditional: falseand encountered no issues, but this is technically an unsound optimization as both flow and typescript have sources of unsoundness:TValue | undefined(typescript's noUncheckedIndexedAccess)anyNote that removing this optimization results in less granular dependencies for ~3% of files on a large Meta project (link).