Disallow reading context during useMemo etc #14653
Conversation
This reverts commit 5fce648.
Mentioned by @acdlite to watch out for
gaearon
force-pushed
the
revert-14651-revert-14648-ctx-bad
branch
from
700680a
to
7cb26fb
Compare
|
ReactDOM: size: -0.1%, gzip: -0.1% Details of bundled changes.Comparing: 5fce648...223960e react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
gaearon
changed the title
|
I pushed another commit that introduces stashing to NewContext so that we don't need to rely on @acdlite Please let me know if this is what you meant — and whether the edge case test I added in 093c8a2 and 7cb26fb is sufficient. (I couldn't get it to fail with the old approach so I probably still misunderstand the issue.) |
Addresses @acdlite's concerns
gaearon
force-pushed
the
revert-14651-revert-14648-ctx-bad
branch
from
f0a8b44
to
cfbdf90
Compare
|
This looks good to me, gimme a few moments to try to break it and then I'll stamp |
There was a problem hiding this comment.
Ok looks good!
I think we need a guard here, too:
It's definitely an edge case. I think it would only happen if you called setState during the render phase of a class component, and the class component was in a different context tree than the component you're updating.
|
Also that reminds me we need a follow-up at some point to add the same guard to class component render phase functions. (Not super urgent, IMO, since that's already in master.) |
|
The same guard being what exactly? I thought we already can’t call Hooks from classes. |
|
You can call readContext though |
|
Right but when (during class render) would we want to disable it? It’s already disabled outside of class render. |
|
In other words — I thought the whole point of readContext is it should work in classes. |
|
Not in a setState reducer though |
|
Aaah. |
|
What makes |
gaearon
force-pushed
the
revert-14651-revert-14648-ctx-bad
branch
from
005e057
to
223960e
Compare
|
Left comment on the PR: #14670 (comment) |
|
I think even this is too much for prod. Hooks are about twice as slow as they should be already and this is adding to a bunch of micro optimizing we need to do. In a better typed form we can guarantee that this never happens. The best way we have to ensure that right now is warnings. As long as we can warn in all cases someone does this, I think it’s fine to leave it in production code since at worst you’re getting what you expected anyway. |
|
Should I... revert? :-) |
|
Well you could land this and follow up by making all the stash things set a flag that triggers a warning in dev. :) |
|
What about "can't call Hooks from inside |
|
I’m most concerned about useMemo and the initial state/action calls in useState/useReducer since those happens just automatically during render. They’re not trigger by an update. So they scale up by the size of the render tree. I haven’t vetted all the reducer callsites if they only happen if there is an actual update in the queue or if they always happen. But for consistency we might as well make them all DEV only. |
…throws I introduced this in facebook#14653 (223960e). But I relied on restart to clean it up in case of error, and didn't notice the try/catch. This makes sure the failing case also restores the current variable which renderWitHooks() currently relies on.
|
Making it all DEV-only in #14677 |
* Revert "Revert "Double-render function components with Hooks in DEV in StrictMode" (facebook#14652)" This reverts commit 3fbebb2. * Revert "Revert "Disallow reading context during useMemo etc" (facebook#14651)" This reverts commit 5fce648. * Add extra passing test for an edge case Mentioned by @acdlite to watch out for * More convoluted test * Don't rely on expirationTime Addresses @acdlite's concerns * Edge case: forbid readContext() during eager reducer
Reverts #14651
In case we want to get it in