Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selectively hydrate during capture phase #22440

Closed
wants to merge 3 commits into from
Closed

Selectively hydrate during capture phase #22440

wants to merge 3 commits into from

Conversation

salazarm
Copy link
Contributor

Summary

This PR adds selective hydration in the capture phase of an event. This makes it so that we run react on*Capture listeners after hydrating.

How did you test this change?

Jest

@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Sep 27, 2021
@sizebot
Copy link

sizebot commented Sep 27, 2021

Comparing: f35287d...ae5f6a3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 127.39 kB 127.34 kB = 40.59 kB 40.56 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.21 kB 130.16 kB = 41.51 kB 41.48 kB
facebook-www/ReactDOM-prod.classic.js = 405.44 kB 405.19 kB = 75.05 kB 75.00 kB
facebook-www/ReactDOM-prod.modern.js = 394.03 kB 393.77 kB = 73.34 kB 73.28 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.44 kB 405.19 kB = 75.05 kB 75.00 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ae5f6a3

@@ -25,4 +25,4 @@ export const SHOULD_NOT_DEFER_CLICK_FOR_FB_SUPPORT_MODE =
// will result in endless cycles like an infinite loop.
// We also don't want to defer during event replaying.
export const SHOULD_NOT_PROCESS_POLYFILL_EVENT_PLUGINS =
IS_EVENT_HANDLE_NON_MANAGED_NODE | IS_NON_DELEGATED | IS_CAPTURE_PHASE;
IS_EVENT_HANDLE_NON_MANAGED_NODE | IS_NON_DELEGATED;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little concerned based off this comment here:

// We don't process these events unless we are in the

Not sure what the effect on those plugins would be or how to test they dont act up? @gaearon any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I see some failing tests I'll take a look at those first

@sebmarkbage
Copy link
Collaborator

I doubt this is safe in this way. It seemed more complicated last I looked at it. Since the next step is to remove the replaying, what's the point of this intermediate step? We might still need it for continuous but the code structure might look different at that point anyway so it might be easier to do then.

@salazarm salazarm closed this Sep 27, 2021
@salazarm salazarm deleted the moveSyncHydrationToCapturePhaseListener branch September 27, 2021 22:56
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.

None yet

4 participants