[Flare] Update interactiveUpdates flushing heuristics #15687
Conversation
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2% Details of bundled changes.Comparing: 31487dd...52ffb56 react-dom
react-native-renderer
react-reconciler
Generated by 🚫 dangerJS |
trueadm
changed the title
|
Also how confident are we that this works in non-Chrome browsers? |
|
@acdlite I checked it Safari, FF, Chrome and Edge. All looking good with this change. I need to confirm with IE, but this is behind a flag anyway, so there should be no immediate risk. |
|
|
||
| dispatchEvent('pointerdown', 100); | ||
| dispatchEvent('pointerup', 100); | ||
| // Ensure the timeStamp logic works |
There was a problem hiding this comment.
Is the Press event component the best place to test this logic?
There was a problem hiding this comment.
It’s fine for now. All responders should have some variant of this too.
There was a problem hiding this comment.
It's that the changes are being made to the core but the tests are here. And those tests have __DEV__-specific results
There was a problem hiding this comment.
We can follow up after and make tests in core too. I don't want to block this any longer given I'm on PTO in a few days. Maybe we can add this to our TODO list otherwise.
|
Can you add a test for: ReactDOM.unstable_batchedUpdates(() => {
// This should flush synchronously
ReactDOM.unstable_flushDiscreteUpdates();
setState();
});Or replace function onClick() {
// This should flush synchronously
ReactDOM.unstable_flushDiscreteUpdates();
setState();
};I believe your current implementation of |
|
Might also want to add a test case that includes an imperative |
|
Per our chat thread, I'd also be OK with adding a TODO above the early return for |
|
I've added more tests, updated tests, added |
| // TODO: Should we fire a warning? This happens if you synchronously invoke | ||
| // an input event inside an effect, like with `element.click()`. | ||
| export function flushDiscreteUpdates() { | ||
| if (workPhase === CommitPhase || workPhase === BatchedPhase) { |
There was a problem hiding this comment.
Why is this a no-op in batchedUpdates? Is there a test that failed otherwise?
There was a problem hiding this comment.
Yep, a bunch of tests failed from memory.
There was a problem hiding this comment.
Can you put a TODO there? I'll take a look later. Doesn't have to block merge.
There was a problem hiding this comment.
Added a TODO. Let's resync with this once I'm back from PTO; this at least unblocks us internally.
|
I've previously encountered some issues related to Event.timeStamp in Vue with forks of older Chrome versions (specifically QtWebEngine) where the timeStamp would be some very large negative number. Might be interesting to take a look at these: vuejs/vue#9681 |
|
I saw some prior issues in Firefox, where the time stamps where using different precision timers (this longer numbers for some events than others). However, unlike Vue, we only care that the time stamps match or do not match – so their precision doesn't matter (even if they are negative). If the timeStamp is 0, we bail back to less optimal behaviour. |
|
^ We need to think about this heuristic more before we ship this to open source but since this is gated behind the Flare feature flag, it's good to merge so we can test it internally. |
This PR changes the logic that occurs in
interactiveUpdates, to account for a scenario that proved problematic in the testing of React Flare – the mixing of both event systems on the same app. Note: this change is behind the experimental event API. Also included in this PR are:interactiveUpdates->discreteUpdatesflushInteractiveUpdates->flushDiscreteUpdatesbatchedEventUpdatesand a newBatchedEventPhaseto schedulerDiscrete events use timeStamp to manage flushing
Now that the Flare event system and the existing event system co-exists, we've noticed far more flushing is occurring; which is problematic for cases where this behaviour was non-existant with only the existing event system. To counter this a
timeStampheuristic has been added by reading thetimeStampfrom thewindowobject. We will probably want to change this to be an argument (rather than reading from window) in the future, but I didn't want to introduce too many changes in this PR.The
timeStampis an important field on events, as it allows us to tell what events were part of the same real-world interaction. For example:mouseup,pointerupandclickall should have the sametimeStamp. If we know that they're all the sametimeStamp, we can do a single flush for them all, which improves compatibility with Flare and also should improve runtime performance as we're flushing sync less.