Double Invoke Effects in __DEV__ #19523
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b9b5750:
|
Details of bundled changes.Comparing: a99bf5c...b9b5750 react-dom
Size changes (stable) |
Details of bundled changes.Comparing: a99bf5c...b9b5750 react-dom
Size changes (experimental) |
packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js
Outdated
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js
Outdated
Show resolved
Hide resolved
|
It can cause some issues. For example, in where I work we have a component that have this kind of effect: React.useEffect(() => {
navigator.clipboard.readText().then(/* …And content to state variable */);
}, []);Also some components that sends analytics / fetch some data, which will probably break many tests. EDIT: |
I'm not sure if this is what you're saying, but you probably shouldn't be sending analytics in DEV mode (or you should be sending them somewhere different than production analytics). As for your concern about testing, that's something I haven't thought much about. I can see where this might cause hassle. |
|
@bvaughn Well, the analytics is not about sending them. We have a logger of analytics (to browsers devtools console), so while developing we can make sure they logged in correct scenarios. If sometimes it called twice and sometimes once, we can't predict if analytics correct. |
|
This looks like it might brake a lot of code. This changes implies that effects can only be used when they have very specific semantics: they should either be idempotent or reversible. Also it makes it a requirement to implement a cleanup even if it is never supposed to run. |
|
The change is fully behind a flag and does not affect any of the code on npm. Indeed, it is likely to break some code, so we want to try it at FB to see how large the impact is. We appreciate your concern but researching this more is the whole point of this PR. Hope this makes sense. |
|
I'll add though that |
lunaruan
force-pushed
the
double_invoke
branch
3 times, most recently
from
fe64e03
to
e51e75b
Compare
|
Ready for review! |
lunaruan
force-pushed
the
double_invoke
branch
from
e51e75b
to
8cee981
Compare
packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js
Outdated
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js
Outdated
Show resolved
Hide resolved
lunaruan
force-pushed
the
double_invoke
branch
from
8cee981
to
b4f716a
Compare
|
Updated the PR based on everyone's comments! |
| let fiber = firstChild; | ||
| while (fiber !== null) { | ||
| if (fiber.child !== null) { | ||
| // Should we add a separate subtree tag for this? |
There was a problem hiding this comment.
What's this comment for? I'm not sure I understand it.
Is this pointing out the fact that we're going to end up traversing all of the subtree paths that have layout effects even if they're all updates and none of them are mounts? So we could add a DEV-only tag e.g. MountLayoutSubtreeTag or something.
Maybe we could add some additional clarification to the wording here.
There was a problem hiding this comment.
Yeah, I wasn't sure how OK it was to add a subtree tag vs. just traversing extra in DEV since it takes extra bytes to add an extra subtreetag, and I know at least with effectTag, we don't seem to want to add too many. If it's in DEV though it makes sense that it's OK. Thanks for clarifying!
| case ForwardRef: | ||
| case SimpleMemoComponent: | ||
| case Block: { | ||
| invokeGuardedCallback( |
There was a problem hiding this comment.
Technically, after https://github.com/facebook/react/pull/19605/files this will be unnecessary since commitHookEffectListUnmount should be safe now.
It's an awkward inconsistency since the equivalent mount isn't safe. I don't know what the answer is.
There was a problem hiding this comment.
Hmm I'll leave it as is for now for consistency and maybe do a separate PR to make everything more consistent (Will creating a safelyCallCreate function cause perf issues)?
This PR double invokes effects in
__DEV__mode.We are thinking about unmounting layout and/or passive effects for a hidden tree. To understand potential issues with this, we want to double invoke effects. This PR changes the behavior in DEV when an effect runs from
create()tocreate() -> destroy() -> create(). The effect cleanup function will still be called before the effect runs in both dev and prod. (Note: This change is purely for research for now as it is likely to break real code.)Note: The change is fully behind a flag and does not affect any of the code on npm.