[Alt] Replay capture phase for continuous events#22856
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 44f99d7...b985780 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
ugh for some reason my comments only show up on commit itself (3c1c876) |
|
I don't think the capture_phase check matters because we call stopPropagation anyways |
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
force-pushed
the
capture-stuff
branch
2 times, most recently
from
c8dd45c to
576fb45
Compare
| nativeEvent.stopPropagation(); | ||
| return; | ||
| } | ||
| return; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: Dan Abramov <dan.abramov@me.com>
gaearon
force-pushed
the
capture-stuff
branch
from
576fb45 to
b985780
Compare
| ); | ||
| if (nextBlockedOn === null) { | ||
| // This whole function is in !enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay, | ||
| // so we don't need the new replay behavior code branch. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Weird, there should be coverage for this block I'll double check.
There was a problem hiding this comment.
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.
Co-authored-by: Dan Abramov <dan.abramov@me.com> Co-authored-by: Marco Salazar <salazarm@fb.com>
4 participants
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.)