Make all readContext() and Hook-in-a-Hook checks DEV-only #14677
Conversation
gaearon
commented
Jan 23, 2019
•
gaearon
commented
- Changes it to be DEV-only
- Implements it for SSR
- Adds a similar DEV-only check for render phase class updates
- Changes all Hooks-in-Hooks warnings to similarly be DEV-only, removing the flaky current variable reassignment that led to bugs (includes a regression test)
|
ReactDOM: size: -0.2%, gzip: -0.1% Details of bundled changes.Comparing: 6cb2677...70c68a2 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
Make readContext() inside Hooks check DEV-only
gaearon
changed the title
|
Ready for review. |
gaearon
force-pushed
the
readcontext-warning
branch
from
9a00d2c to
3e4949d
Compare
gaearon
force-pushed
the
readcontext-warning
branch
from
3e4949d to
7085e70
Compare
There was a problem hiding this comment.
So there is another technique we have for hooks. If you think about them as algebraic effects, how do you control the implementation within various contexts?
You change the current dispatcher.
I believe we did that in some other place.
If you wrote a custom dispatcher which wrapped useContext/readContext, in DEV, what would else be different on that dispatcher?
Would you also disallow other hooks in the same scope?
| currentlyRenderingFiber = null; | ||
| lastContextDependency = null; | ||
| lastContextWithAllBitsObserved = null; | ||
| export function stashContextDependenciesInDEV(): void { |
There was a problem hiding this comment.
These names don’t really make sense anymore.
How about enter/exitDisallowedContextRead?
|
A separate dispatcher sounds like a cleaner way to do it. I initially thought of replacing the dispatcher while we're inside a Hook like |
|
I don't understand what you mean by "other hooks in the same scope". |
|
I think the way we allow currentlyRenderingFiber to be null is also wrong. That should also be done with the dispatcher technique. I have a hard time reviewing these because it’s so spread out. If we’re confident that doing this whole refactor after 16.8, it’s fine but I can’t tell if that’s a breaking change or not from the code. |
|
For the currentlyRenderingFiber thing. It should always be non-null when the dispatcher is set because we’re always rendering something. We should instead swap out the whole dispatcher when we’re inside user code. That way we also remove the whole resolveRenderingThingy() call in each hook. |
|
By “Other hooks in the same scope” I mean, it’s not clear to me from the code which hooks are disallowed and in what user code. Is it that most user code is covered by the “currentlyRenderingFiber === null” check and this is adding something additional or are there scopes where either one is in effect but not both? |
|
I think there's some confusion here. Here's a brief history of this code:
|
Before #14608
Between #14608 and This PR
This PR
|
…4677) * Make readContext() in Hooks DEV-only warning * Warn about readContext() during class render-phase setState() * Warn on readContext() in SSR inside useMemo and useReducer * Make all Hooks-in-Hooks warnings DEV-only * Rename stashContextDependencies * Clean up warning state on errors
