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

API for prerendering a top-level update and deferring the commit #11232

Closed
wants to merge 2 commits into from

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 16, 2017

Adds the ability to start rendering work without flushing the changes to the screen, by blocking the commit phase. This can be used to coordinate React's commit phase with other async work.

  • root.prerender schedules an update and returns a work object.
  • work.commit unblocks the commit phase and flushes the remaining
    work synchronously.

(Not yet implemented is work.then, which schedules a callback that fires once the work has completed. To keep these PRs focused, I'll do this in a follow-up.)

Adds the ability to start rendering work without flushing the changes
to the screen, by blocking the commit phase. This can be used to
coordinate React's commit phase with other async work.

- root.prerender schedules an update and returns a work object.
- work.commit unblocks the commit phase and flushes the remaining
  work synchronously.

(Not yet implemented is work.then, which schedules a callback that
fires once the work has completed.)
Implements a limited version of resuming that only applies to the root,
to account for the pathological case where a prerendered root must be
completely restarted before it can commit. This won't be necessary
once we implement resuming for real.
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'd be ok with landing this approach on isBlocked as a temporary incremental step but I really think we need to revisit why the normal API can't be implement on top these details, and therefore let us remove the concept of "blocked".

DOMRenderer.unblockRoot(root, expirationTime);
DOMRenderer.flushRoot(root, expirationTime);
};

type ReactRootNode = {
render(children: ReactNodeList, callback: ?() => mixed): void,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this new API really should return a Thenable since that's what people have been asking for with these callback APIs. I noticed because the prerender API doesn't have it.

// expiration time.
// TODO: Can this be unified with updateContainer? Or is it incompatible
// with the existing semantics of ReactDOM.render?
updateRoot(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to be stricter with naming of these things. I'm not sure what makes most sense here since Root is otherwise just an internal concept. I think this still make sense to refer to Container here. From the outside perspective there is no root.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Especially since the comment eve says that this might not even be a root.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't we just add an API called createRoot? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also I think that comment is outdated post-portals, but I wasn't sure enough to delete it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the createRoot for persistent updates, HostRoot fiber and createRoot in the ReactDOM renderer. Neither of which are the same and neither is this.

updateRoot(
element: ReactNodeList,
container: OpaqueRoot,
parentComponent: ?React$Component<any, any>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is an new API we don't have to support legacy apis like renderSubtreeIntoContainer.

return scheduleTopLevelUpdate(current, element, true, callback);
},

unblockRoot(root: OpaqueRoot, expirationTime: ExpirationTime) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need these to be separate? This is inherent complexity to support where as the current API only lets you do both at once.

I think we should try again to simply defer all commits until explicitly called since we have so many cases where that seems to matter. And the legacy mode is simply a wrapper that does that eagerly. In that case that's the normal mode so the concept of being blocked doesn't make sense and unblocking doesn't as well.

type WorkNode = {
commit(): void,

_reactRootContainer: *,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use an existential type here instead of something explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I was lazy and didn't want to import it :D

Copy link

Choose a reason for hiding this comment

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

:D

@acdlite
Copy link
Collaborator Author

acdlite commented Oct 24, 2017

Closing in favor of new approach

@acdlite acdlite closed this Oct 24, 2017
@acdlite
Copy link
Collaborator Author

acdlite commented Oct 24, 2017

New PR #11346

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.

5 participants