Simplify nested hook warning logic in dev #26238
Open
Conversation
This file contains 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
We have so many dispatchers. And with my changes in facebook#26232 it's a bit harder to see if the previous logic was correct. Here I'm moving the flag to a separate value which makes it easier to see that it will work properly with the live dispatcher switch and that the prod behavior is not affected. I cheated and mutated ContextOnlyDispatcher in place in dev (previously ContextOnlyDispatcher.readContext would not warn when in a state update but that dispatcher wouldn't be active at the times it's relevant). Also adds the hook ordering warnings to useMemoCache.
facebook-github-bot
added
CLA Signed
React Core Team
Comment on lines
+2924
to
+2929
|
|
||
| ContextOnlyDispatcher.readContext = function <T>( | ||
| context: ReactContext<T>, | ||
| ): T { | ||
| warnInvalidContextAccess(); | ||
| return readContext(context); |
There was a problem hiding this comment.
Cannot assign function to
ContextOnlyDispatcher.readContextbecause propertyreadContextis not writable.
One of the ways to tackle this will be to declare warnIfDevInvalidContextAccess fn such that:
- it'll behave as warn-IfDev-InvalidContextAccess
- which may use
__DEV__(?) on top of current behaviour ofwarnInvalidContextAccess - and to declare it in the adjacent scope of
ContextOnlyDispatcher
and declaring ContextOnlyDispatcher as:
export const ContextOnlyDispatcher: Dispatcher = {
readContext: function <T>(
context: ReactContext<T>,
): T {
warnIfDevInvalidContextAccess();// the above refactored fn
return readContext(context);
},
...,
}
|
@acdlite thoughts on this PR? (getting rid of the InvalidNestedHooksDispatcherOn{Mount,Update,Rerender}InDEV dispatchers in favor of tracking a separate flag that the other dev dispatchers read) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have so many dispatchers. And with my changes in #26232 it's a bit harder to see if the previous logic was correct. Here I'm moving the flag to a separate value which makes it easier to see that it will work properly with the live dispatcher switch and that the prod behavior is not affected.
I cheated and mutated ContextOnlyDispatcher in place in dev (previously ContextOnlyDispatcher.readContext would not warn when in a state update but that dispatcher wouldn't be active at the times it's relevant).
Also adds the hook ordering warnings to useMemoCache.