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

[Alt] Replay capture phase for continuous events #22856

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 2, 2021

This is basically #22680 (tests are copypasta), split into smaller pieces. There's a bit less abstraction but all important changes should be the same. This PR is scoped to behavior changes. It depends on stacked refactorings.

All behavior changes are behind flag-only branches. (Assuming earlier refactorings are safe.)

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 2, 2021
@sizebot
Copy link

sizebot commented Dec 2, 2021

Comparing: 44f99d7...b985780

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 = 130.22 kB 130.22 kB = 41.64 kB 41.64 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.97 kB 134.97 kB = 43.02 kB 43.02 kB
facebook-www/ReactDOM-prod.classic.js = 427.52 kB 426.95 kB = 78.48 kB 78.45 kB
facebook-www/ReactDOM-prod.modern.js = 416.08 kB 415.51 kB = 76.82 kB 76.77 kB
facebook-www/ReactDOMForked-prod.classic.js = 427.52 kB 426.95 kB = 78.48 kB 78.45 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b985780

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 2, 2021

ugh for some reason my comments only show up on commit itself (3c1c876)

@salazarm
Copy link
Contributor

salazarm commented Dec 2, 2021

I don't think the capture_phase check matters because we call stopPropagation anyways

@salazarm
Copy link
Contributor

salazarm commented Dec 2, 2021

This branch is for queued discrete events. So I think it's actually unnecessary and maybe should be deleted?

We can do that in a follow-up diff. I've been testing not queueing discrete events for a month and i think we should be good to delete that old behavior now since everything seems neutral still.

@gaearon gaearon force-pushed the capture-stuff branch 2 times, most recently from c8dd45c to 576fb45 Compare December 2, 2021 14:19
}
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm worried that removing this return doesn't break any tests. I think it's correct to have it but we should follow-up to have coverage against over-dispatching.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the real problem is that removing lines 341 - 347:

dispatchEventForPluginEventSystem(
    domEventName,
    eventSystemFlags,
    nativeEvent,
    null,
    targetContainer,
  );

Didn't cause any tests to fail. Honestly, tracing through the code, dispatching with a null target does not seem to accomplish anything since it would lead to 0 listeners being executed O.o

@@ -532,6 +543,8 @@ function replayUnblockedEvents() {
nextDiscreteEvent.nativeEvent,
);
if (nextBlockedOn === null) {
// This whole function is in !enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay,
// so we don't need the new replay behavior code branch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like that I'm able to remove the contents of this whole if statement and none of the tests seem to fail, even if I hardcode enableClientRenderFallbackOnHydrationMismatch in ReactFeatureFlags.www-dynamic to false.

It would be nice to have a test verifying this does something. Though it is old behavior, I feel uncomfortable it's not tested until it's deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, there should be coverage for this block I'll double check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think I'm just going to delete it now since we've been testing with discrete event replay off for a while so I'll put up a PR for that.

@gaearon gaearon merged commit 71d1675 into facebook:main Dec 2, 2021
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
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.

4 participants