Skip to content

Conversation

@tyao1
Copy link
Contributor

@tyao1 tyao1 commented Dec 21, 2022

Summary

If there is a click event following a series of mouse over events, we want the callback in the click event to be able to read the latest states set by mouse over.

How did you test this change?

yarn test

@tyao1 tyao1 requested a review from acdlite December 21, 2022 02:47
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 21, 2022
) {
const root: ?FiberRoot = node.stateNode;
if (root != null) {
flushPendingContinuousUpdates(root);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too late because the changes we're flushing could have affected the things you read above here. E.g. the target could've been removed.

However, I think technically, all our events are too late because I believe that we only attach the listeners at the React root. For example, if a user space event listener attached in a useEffect is listening to a parent above the React root like the document, then that thing should probably observe it too even though it's dispatched before the capture of the React root.

I think this is already a problem for selective hydration too.

Keep in mind that this whole synthetic event system is on its way out and so anything that relies on it won't keep working. Instead, think of all events being dispatched through the native DOM events and how that would work.

As a result, I would recommend not trying to fit this into the existing event system but instead add your own event listeners that do this flushing. That way this is kept separate and this is not deleted when we delete the rest of the synthetic event system.

E.g. here you could listen at the DOM root.

https://github.com/facebook/react/blob/main/packages/react-dom/src/client/ReactDOMRoot.js#L256

Float already has a concept of figuring out the root of the document outside the React root.

https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactDOMFloatClient.js#L193

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context and code pointers!

@shreya-as
Copy link

shreya-as commented Jan 16, 2023

<--!test -->
test

Copy link

@sennymunoz sennymunoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realy

@tyao1 tyao1 force-pushed the ty-flush-continuous-before-discrete branch from 4203dc6 to c2cebbf Compare January 21, 2023 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants