[Partial Hydration] Dispatching events should not work until hydration commits #16532
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
This is equivalent to a "Placement" effect in that it's a new insertion to the tree but it doesn't need an actual mutation. It is only used to determine if a subtree has actually mounted yet.
Previous roots had a Placement flag on them as a hack for this case but since we have a special flag for it now, we can just use that.
sebmarkbage
commented
Aug 21, 2019
|
|
||
| // We're now partially hydrated. | ||
| a.click(); | ||
| expect(clicks).toBe(0); |
There was a problem hiding this comment.
This is the thing that this PR fixes. It used to let these through.
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: efa5dbe...93bc6cb react-art
react-dom
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
|
Please can you add a test for the new event system? You could add a test in |
|
@trueadm That seems like a bigger issue we need to deal with. Particularly, this is not concurrent mode safe: https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberCompleteWork.js#L1346-L1347 |
|
That code was designed to work like the existing model for assigning props against the DOM instance, which is also done in the complete phase. I’m happy to change it, but it’s based on previous feedback that I guess no longer holds with concurrent mode? |
|
@trueadm It assigned against the DOM in the complete phase for initial mount (when there is no current). That normally work since you can't fire an event on something that doesn't exist in the DOM yet (except for hydration). That's the same thing that this PR is dealing with. That part is legit, so I'll add a test for the same thing for Flare. (I believe it should just work since Flare goes through the same mechanism in However, for updates to an already mounted DOM component we do it in the commit phase since the old listeners remain valid until we're ready to commit. https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMHostConfig.js#L371 |
|
Seems like those two lines you highlighted need to be moved to the commit phase then and then it should work as expected. I can do that tomorrow :) |
robertistok
approved these changes
Aug 22, 2019
trueadm
approved these changes
Aug 22, 2019
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.
This includes a bunch of clean up to set up for selective hydration.
Primarily the bug it fixes is that we attach event listeners while hydrating but these need to be noops (and later replayable) until we actually commit. Since the parent may not be mounted any setStates that we let through might become invalid.
There's no way around that we need some commit-phase effect to indicate when it's done. We could do these on all event listeners but it's sufficient to just do it at the root of the things we're hydrating.
In fact, we did that with HostRoot already by attaching a fake Placement effect tag on it.
This PR formalizes that by making Hydrating a new effect tag. It means that this is the beginning of a in-progress hydrating subtree. The tag gets removed in the commit phase to indicate that the tree is mounted. Same as Placement.