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

root.createBatch #11473

Merged
merged 1 commit into from
Nov 29, 2017
Merged

root.createBatch #11473

merged 1 commit into from
Nov 29, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 6, 2017

Alternative to #11346

API for batching top-level updates and deferring the commit.

  • root.createBatch creates a batch with an async expiration time associated with it.
  • batch.render updates the children that the batch renders.
  • batch.then resolves when the root has completed.
  • batch.commit synchronously flushes any remaining work and commits.

No two batches can have the same expiration time. The only way to commit a batch is by calling its commit method. E.g. flushing one batch will not cause a different batch to also flush.

TODO: Specify error handling behavior.

@sebmarkbage
Copy link
Collaborator

Can you remind me why you didn't think the createBatch API was good and why you changed your mind?

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 7, 2017

@sebmarkbage

The main problem with my prerender PR (#11346) is that if you prerender multiple times, committing one update has the effect of also committing updates at the same or earlier expiration time, removing the guarantee that an update won't be flushed until the commit method is explicitly called:

const work1 = root.prerender('a');
work1.then(onComplete1);

const work2 = root.prerender('b');
work2.then(onComplete2);

// Flush all work up to and including work2
work2.commit();
// Should onComplete1 have fired? Should work1 be aborted? Unclear.

The createBatch API solves this by assigning a unique expiration time to every batch. If a later batch is committed, its expiration time is upgraded such that it's the earliest batch. This way, committing one batch never commits any other batch, and the only way to commit a batch is by calling its commit method.

But you could do this with the prerender API, too, where each call to prerender produces a unique expiration time.

However, there are valid reasons why you might want to update a root twice at the same expiration time, like if you receive new props from the server before the previous props have completed. This isn't possible using the prerender API in a way that also satisfies the previous constraint (each update commits separately).

The createBatch API gives you both abilities: separate batches commit separately, but updates to the same batch are batched:

const batch1 = root.createBatch();
batch1.render('a');
batch1.then(onComplete1);

const batch2 = root.createBatch();
batch2.render('b');
batch2.then(onComplete2);
// Update again at the same expiration time
batch2.render('c');

// Both `b` and `c` are flushed
// onComplete2 fires, but not onComplete1
batch2.commit();

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I think this makes sense but I'm going to need a moment to get comfortable with the amount of code that's here. Is there any thing we can do to hoist things out into a separate module?

let expirationTime;
// Check if the top-level element is an async wrapper component. If so,
// treat updates to the root as async. This is a bit weird but lets us
// avoid a separate `renderAsync` API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

😃

This comment doesn't really make sense now does it?

@sebmarkbage
Copy link
Collaborator

What's the actual file size impact?

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 17, 2017

@sebmarkbage I think the whole ReactBatch class could get pulled out into a separate module in a straightforward way: https://github.com/acdlite/react/blob/df28e9a61163e2107aa186922f1e1e534f5cc853/packages/react-dom/src/client/ReactDOM.js#L184-L318

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 17, 2017

As this PR currently stands, it increases the ReactDOM prod bundle from 30.08 KB -> 31.3 KB

@sebmarkbage
Copy link
Collaborator

That's like 0.3 preacts. That's quite significant for something that doesn't even do the actual work but is just plumbing.

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 21, 2017

I think I can get it down a bit but the best way to offset it would be to remove legacy edge cases.

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 21, 2017

Or move it into a separate package, but if this is the way to do async, then I don't know how much that really helps medium to long term.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Alright, let's see if we can optimize it when it is in place.

It would nice to at least be able to DCE if the export is never referenced, and also the inverse. If the legacy APIs are never referenced.

@blling
Copy link

blling commented Nov 28, 2017

What is DCE?

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

dead code elimination

API for batching top-level updates and deferring the commit.

- `root.createBatch` creates a batch with an async expiration time
  associated with it.
- `batch.render` updates the children that the batch renders.
- `batch.then` resolves when the root has completed.
- `batch.commit` synchronously flushes any remaining work and commits.

No two batches can have the same expiration time. The only way to
commit a batch is by calling its `commit` method. E.g. flushing one
batch will not cause a different batch to also flush.
@NE-SmallTown
Copy link
Contributor

@acdlite Can I ask that what's the different between createBatch -> render -> commit and createRoot -> render -> flush ? Seems that they both implement the same effect

@NE-SmallTown
Copy link
Contributor

 it('can defer a commit by batching it', () => {
   const root = ReactDOM.createRoot(container);
   const batch = root.createBatch();
   batch.render(<div>Hi</div>);
   // Hasn't committed yet
   expect(container.textContent).toEqual('');
    // Commit
    batch.commit();
  expect(container.textContent).toEqual('Hi');
});

I think the word 'defer' is not correct, event I don't use createBatch and just replace batch.render with root.render and batch.commit with flush(), the result is the same.

@NE-SmallTown
Copy link
Contributor

Another question is that it seems createBatch has side effect, i.e. :

 const root = ReactDOM.unstable_createRoot(container);
    // if I add this line, the Foo won't be render and work.then won't be called after call flush()
    // const batch = root.createBatch();

    function Foo(props) {
      console.log('Foo render!');
      return props.children;
    }

    const work = root.render(<Foo>Foo</Foo>);

    // flush();
    console.log(11, container.textContent);

    // Hasn't updated yet
    expect(container.textContent).toEqual('');

    work.then(() => {
      console.log(22, container.textContent);
    });

    flush();

NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
API for batching top-level updates and deferring the commit.

- `root.createBatch` creates a batch with an async expiration time
  associated with it.
- `batch.render` updates the children that the batch renders.
- `batch.then` resolves when the root has completed.
- `batch.commit` synchronously flushes any remaining work and commits.

No two batches can have the same expiration time. The only way to
commit a batch is by calling its `commit` method. E.g. flushing one
batch will not cause a different batch to also flush.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants