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

chore(blocks.render): mutex for onchange added to the blocks.render() #2449

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

neSpecc
Copy link
Member

@neSpecc neSpecc commented Aug 20, 2023

TL;DR

onChange won't be called on blocks.render(). Like in 2.27 and before.

Context

#2430 introduced a few updates:

  • onChange mutex was removed from Renderer since Renderer.render() became awaitable and we just enable Modification Observer after initial rendering:

this.validate();
this.init();
await this.start();
await this.render();
const { BlockManager, Caret, UI, ModificationsObserver } = this.moduleInstances;
UI.checkEmptiness();
ModificationsObserver.enable();

So the onChange became enabled in the blocks.render() API

  • blocks.clear() now triggers onChange with removed blocks
    public async clear(needToAddDefaultBlock = false): Promise<void> {
    const queue = new PromiseQueue();
    this.blocks.forEach((block) => {
    queue.add(async () => {
    await this.removeBlock(block, false);
    });
    });
    await queue.completed;

Both of these changes are correct and actual. The initial render does not call onChange (because a document is not changed), but the blocks.render() calls it with removed and added blocks (because an existing document is cleared).

Problem

During the RC test we got feedback that the blocks.render() method is expected to be mutation-free.

Solution

I thought about it and found three solutions:

  • add "silent" option to the render() method
  • add new API for the onChange enabling/disabling
  • just mute onChange inside the render()

After all, I've decided to mute onChange inside the render method for two pros:

  • it is the same behavior that we have in current version 2.27, so 2.28 won't introduce a breaking change here.
  • semantic meaning of the render() method is to display a new document instead of a current that stays unchanged.

Cons:

  • It's not obvious that some methods lead onChange call, but some are not. I think that we need to redesign the API at all, based on the new Document Model in 3.0.

Also

blocks.clear() now returns a promise. So we can call it before render as a workaround if we want to receive "block-removed" events

await editor.blocks.clear(); // onChange will be called
await editor.blocks.render();

@neSpecc neSpecc merged commit 42612a0 into next Aug 21, 2023
6 checks passed
@neSpecc neSpecc deleted the silent-render branch August 21, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants