-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
[Fast Refresh] Support callthrough HOCs #21104
Conversation
Comparing: 1b7e471...00178db Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
This shows why my initial approach doesn't make sense.
_s(); | ||
const [foo, setFoo] = useState(0); | ||
React.useEffect(() => {}); | ||
return <h1 ref={ref}>{foo}</h1>; | ||
})), "useState{[foo, setFoo](0)}\\nuseEffect{}"); | ||
}, "useState{[foo, setFoo](0)}\\nuseEffect{}")), "useState{[foo, setFoo](0)}\\nuseEffect{}")), "useState{[foo, setFoo](0)}\\nuseEffect{}"); |
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 adds a bunch of duplication to the output. But the real one is hashed. Also it's only for HOCs which are out of favor. So meh.
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.
Also it's only for HOCs which are out of favor. So meh.
Then again, isn't this change itself to support an even more uncommon use case (a HOC like pattern that directly calls a component as a function)?
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.
Well, HOCs are not that uncommon.
In the original case, we have a roughly 7% usage library which downright causes a crash every time a component is edited, in default setups with CRA/Next/RN. Pretty disruptive and they don't have a fix without giving up on the whole approach.
I'm just saying that the added output duplication does not make the remaining cases worse. It just slightly bloats the DEV-only output.
ok I think this approach works |
Great! Can’t wait to try it! |
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 looked at the new tests and the changed snapshots and they look good 👍🏼
I have not thought deeply about what types of edge-case patterns this change might break.
It don’t see how it would break render prop usage. I’d need a more specific example to explain why it doesn’t.
Normally I would want to test FR with these changes against a project with react-virtualized or something that makes use of other direct component calls (via render props) but I think that pattern would already be fundamentally broken b'c hooks wouldn't be allowed inside of a one-to-many render prop scenario like react-virtualized.
That being said, I wrote a test anyway where The row-render-prop functions returned a React element that itself used hooks, and that test failed (but also failed on master) so maybe that's fine 😁
This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place.
This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe.
Let's give this a try. I fixed a corner case I found at FB but haven't seen other issues. Something like #21139 would be a better longer-term fix. |
Should I also implement this for |
Yeah I guess that would be nice. |
_c3 = A; | ||
export const B = React.memo(_c5 = React.forwardRef(_c4 = _s2(function (props, ref) { | ||
export const B = _s2(React.memo(_c5 = _s2(React.forwardRef(_c4 = _s2(function (props, ref) { |
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'm porting this PR to the TypeScript version.
I aggressively detect if the function needs to be hashed appears at the callee position of the CallExpression (which means only literal IIFE ((() => {})(...args)
)) instead of allowing it appears in the argument position (memo(f)
). Is that OK?
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.
Not sure I understand the question.
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.
So in this PR, it seems there are some irrelated (or false positive) changes in the snapshot:
And in the TS version there are no such snapshot changes, only the new test case
while (item) {
;((item) => {
useFoo()
})(item)
}
I'd like to know if those changes are really false positive or I should port those behaviors to make it work correctly?
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'll release it for now
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.
Why do you call these changes false positive? Can you describe what you expected vs what you saw?
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.
So as the PR title describes, it add support for IIFEs. But these baseline changes are not IIFEs, just a normal function call
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 there's a misunderstanding.
The change in this PR has nothing to do with IIFEs. It only relates to higher-order components that call rather than render the passed function.
The IIFE test case is not something that's related to Hooks or Fast Refresh. It's just that we had a case like this in the codebase that was accidentally getting detected as a Hook call (because of the use
prefix). My change broke the semantics of that code. So I added a regression test that verifies that code like this does not get broken.
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.
So what's the actual test case? Can you give a minimal example so I can fix that?
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.
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.
Concretely, the problem was that walking upwards was only correct as long as the inner node represents the argument. If the child node was the callee itself we should stop walking upwards.
Implemented in https://github.com/Jack-Works/react-refresh-transformer/pull/5/files. but I still have some question about this PR posted in #21104 (comment) |
Summary: This sync includes the following changes: - **[c9aab1c9d](facebook/react@c9aab1c9d )**: react-refresh@0.10.0 //<Dan Abramov>// - **[516b76b9a](facebook/react@516b76b9a )**: [Fast Refresh] Support callthrough HOCs ([#21104](facebook/react#21104)) //<Dan Abramov>// - **[0853aab74](facebook/react@0853aab74 )**: Log all errors to console.error by default ([#21130](facebook/react#21130)) //<Sebastian Markbåge>// - **[d1294c9d4](facebook/react@d1294c9d4 )**: Add global onError handler ([#21129](facebook/react#21129)) //<Sebastian Markbåge>// - **[64983aab5](facebook/react@64983aab5 )**: Remove redundant setUpdatePriority call ([#21127](facebook/react#21127)) //<Andrew Clark>// - **[634cc52e6](facebook/react@634cc52e6 )**: Delete dead variable: currentEventWipLanes ([#21123](facebook/react#21123)) //<Andrew Clark>// - **[1102224bb](facebook/react@1102224bb )**: Fix: flushSync changes priority inside effect ([#21122](facebook/react#21122)) //<Andrew Clark>// - **[dbe98a5aa](facebook/react@dbe98a5aa )**: Move sync task queue to its own module ([#21109](facebook/react#21109)) //<Andrew Clark>// - **[3ba5c8737](facebook/react@3ba5c8737 )**: Remove Scheduler indirection ([#21107](facebook/react#21107)) //<Andrew Clark>// - **[46b68eaf6](facebook/react@46b68eaf6 )**: Delete LanePriority type ([#21090](facebook/react#21090)) //<Andrew Clark>// - **[dcd13045e](facebook/react@dcd13045e )**: Use Lane to track root callback priority ([#21089](facebook/react#21089)) //<Andrew Clark>// - **[5f21a9fca](facebook/react@5f21a9fca )**: Clean up host pointers in level 2 of clean-up flag ([#21112](facebook/react#21112)) //<Andrew Clark>// - **[32d6f39ed](facebook/react@32d6f39ed )**: [Fizz] Support special HTML/SVG/MathML tags to suspend ([#21113](facebook/react#21113)) //<Sebastian Markbåge>// - **[a77dd13ed](facebook/react@a77dd13ed )**: Delete enableDiscreteEventFlushingChange ([#21110](facebook/react#21110)) //<Andrew Clark>// - **[048ee4c0c](facebook/react@048ee4c0c )**: Use `act` in fuzz tester to flush expired work ([#21108](facebook/react#21108)) //<Andrew Clark>// - **[556644e23](facebook/react@556644e23 )**: Fix plurals ([#21106](facebook/react#21106)) //<Sebastian Markbåge>// - **[8b741437b](facebook/react@8b741437b )**: Rename SuspendedWork to Task ([#21105](facebook/react#21105)) //<Sebastian Markbåge>// - **[38a1aedb4](facebook/react@38a1aedb4 )**: [Fizz] Add FormatContext and Refactor Work ([#21103](facebook/react#21103)) //<Sebastian Markbåge>// - **[1b7e471b9](facebook/react@1b7e471b9 )**: React Native New Architecture: Support passing nativeViewTag to getInspectorDataForViewAtPoint callback, for React DevTools compat ([#21080](facebook/react#21080)) //<Joshua Gross>// - **[4a99c5c3a](facebook/react@4a99c5c3a )**: Use highest priority lane to detect interruptions ([#21088](facebook/react#21088)) //<Andrew Clark>// - **[77be52729](facebook/react@77be52729 )**: Remove LanePriority from computeExpirationTime ([#21087](facebook/react#21087)) //<Andrew Clark>// - **[3221e8fba](facebook/react@3221e8fba )**: Remove LanePriority from getBumpedLaneForHydration ([#21086](facebook/react#21086)) //<Andrew Clark>// - **[05ec0d764](facebook/react@05ec0d764 )**: Entangled expired lanes with SyncLane ([#21083](facebook/react#21083)) //<Andrew Clark>// - **[03ede83d2](facebook/react@03ede83d2 )**: Use EventPriority to track update priority ([#21082](facebook/react#21082)) //<Andrew Clark>// - **[a63f0953b](facebook/react@a63f0953b )**: Delete SyncBatchedLane ([#21061](facebook/react#21061)) //<Ricky>// - **[fa868d6be](facebook/react@fa868d6be )**: Make opaque EventPriority type a Lane internally ([#21065](facebook/react#21065)) //<Andrew Clark>// - **[eb58c3909](facebook/react@eb58c3909 )**: react-hooks/exhaustive-deps: Handle optional chained methods as dependency ([#20204](facebook/react#20204)) ([#20247](facebook/react#20247)) //<Ari Perkkiö>// - **[7b84dbd16](facebook/react@7b84dbd16 )**: Fail build on deep requires in npm packages ([#21063](facebook/react#21063)) //<Dan Abramov>// - **[2c9d8efc8](facebook/react@2c9d8efc8 )**: Add react-reconciler/constants entry point ([#21062](facebook/react#21062)) //<Dan Abramov>// - **[d0eaf7829](facebook/react@d0eaf7829 )**: Move priorities to separate import to break cycle ([#21060](facebook/react#21060)) //<Andrew Clark>// - **[435cff986](facebook/react@435cff986 )**: [Fizz] Expose callbacks in options for when various stages of the content is done ([#21056](facebook/react#21056)) //<Sebastian Markbåge>// - **[25bfa287f](facebook/react@25bfa287f )**: [Experiment] Add feature flag for more aggressive memory clean-up of deleted fiber trees ([#21039](facebook/react#21039)) //<Benoit Girard>// - **[8fe7810e7](facebook/react@8fe7810e7 )**: Remove already completed comment ([#21054](facebook/react#21054)) //<Sebastian Markbåge>// - **[6c3202b1e](facebook/react@6c3202b1e )**: [Fizz] Use identifierPrefix to avoid conflicts within the same response ([#21037](facebook/react#21037)) //<Sebastian Markbåge>// - **[dcdf8de7e](facebook/react@dcdf8de7e )**: Remove discrete lanes and priorities ([#21040](facebook/react#21040)) //<Andrew Clark>// - **[ca99ae97b](facebook/react@ca99ae97b )**: Replace some flushExpired callsites ([#20975](facebook/react#20975)) //<Ricky>// - **[1fafac002](facebook/react@1fafac002 )**: Use SyncLane for discrete event hydration ([#21038](facebook/react#21038)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 6d3ecb7...c9aab1c jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D27436763 fbshipit-source-id: da79a41e26bffdcdacd293178062edf098e9b58a
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
(Jack-Works#5)", close Jack-Works#2" This reverts commit 9458493.
* [Fast Refresh] Support callthrough HOCs * Add a newly failing testing to demonstrate the flaw This shows why my initial approach doesn't make sense. * Attach signatures at every nesting level * Sign nested memo/forwardRef too * Add an IIFE test This is not a case that is important for Fast Refresh, but we shouldn't change the code semantics. This case shows the transform isn't quite correct. It's wrapping the call at the wrong place. * Find HOCs above more precisely This fixes a false positive that was causing an IIFE to be wrapped in the wrong place, which made the wrapping unsafe. * Be defensive against non-components being passed to setSignature * Fix lint
I'm facing a stranger bug may related with this: vitejs/vite-plugin-react#6 When I change the styled doesn't change anything, I also tried a custom HOC and same thing happen. But when I change the syntax to make the const have the wrapped value of styled it worked.
This is the path output var from the src/components/Pages/Public/Login/index.tsx, as you can see the parentPath.type is VariableDeclarator so it is skip by the if (path.parent.type === 'VariableDeclarator') |
Can you please clarify by posting the exact minimal snippets that (1) work and (2) don't work? |
Sure! // Does not work
const Component =({ className }) => {
return <div className={className}>hi!</div>
}
export default styled(Component)`
background-color: red;
`
// Worked
const Component = styled(({ className }) => {
return <div className={className}>hi!</div>
})`
background-color: red;
`
// Worked
const component =({ className }) => { // <~ lowercase
return <div className={className}>hi!</div>
}
// uppsercase
export default Styled(Component)`
background-color: red;
` |
Here an example: https://stackblitz.com/edit/vitejs-vite-k3bzhu?file=src/Test.jsx Try to change the background-color or try to change the parameter of the TestMyHoc. TestMyHocWorking is an example where refresh is working as expected, I think is because is not a call assignment. |
Does the same happen in CodeSandbox? Would be nice to check to see if this is Vite-specific. |
It's not happening with react-scritps: https://stackblitz.com/edit/react-ptscnt?file=src%2FTest.jsx But when use HOC without a call assignment it works. |
If it's not happening with |
Fixes #20417.
See explanation of the problem in #20417 (comment). The fix I settled on (see next commits) is to add signatures to all HOC wrappers above, not just the inner function. If they already had signatures, they would be ignored. But if not, this is our chance to tag them so that if they call our component as a function directly (instead of rendering it), we take its list of Hooks into account.
This still won't solve the problem for more convoluted cases, like if you're developing a HOC that does callthrough. Because then the transform will for it too, and fill its signature first. But that seems like a reasonable compromise since the whole pattern is shady. Let's just get the MobX case working.