Fix discrete nested updates #6419
Conversation
zurfyx
requested review from
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
| expect(onUpdate).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('can use discrete after a non-discrete setEditorState to flush the entire queue', () => { |
There was a problem hiding this comment.
Is flushing the entire queue correct or should we flush the pending updates, then flush the new update separately? I'm worried about leaking flags and such.
There was a problem hiding this comment.
@acywatson fair point, this is correct as far as the current behavior goes. If you had a discrete update with nested non-discrete updates, the nested updates would also be bundled into the discrete. I think this is worth an offline discussion, clearly nested updates and discrete are very unpredictable.
There was a problem hiding this comment.
Last time I looked at this I couldn't think of a way to do it correctly with the current API.
✅ probably ok, because the outer update doesn't do anything on its own (common in a command listener)
editor.update(() => {
editor.update(() => $changeSomeStuff(), { discrete: true });
// this is fine assuming reconciliation already happened, in which case the rest of the update is more like a read
});✅ probably ok, because the discrete update is a "tail call" so it would be equivalent to floating the discrete flag upwards
editor.update(() => {
$changeSomeStuff();
editor.update(() => $changeSomeStuff(), { discrete: true });
// this is fine assuming reconciliation already happened, in which case the rest of the update is more like a read
});❌ this breaks assumptions about update atomicity
editor.update(() => {
$changeSomeStuff();
editor.update(() => $changeSomeStuff(), { discrete: true });
// oh no! this will be a separate logical update or the discrete update actually hasn't been reconciled yet
$changeSomeStuff();
});There was a problem hiding this comment.
I think the "most correct" workaround for this would be to have some way to mark the outer update as read-only after flush. I think any solution (or non-solution) here is going to be a bit convoluted.
If the
discreteflag was part of a nested update it was being ignored. Instead, the correct behavior is that this update and the previously computed updates should be flushed.