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

Push host root context when bailing out on low priority #10712

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 14, 2017

Prevents a push/pop mismatch when bailing out on HostRoots. This is currently unobservable, because HostRoots never bail out on low priority, but this does happen with prerendering.

I found this when rebasing #10624 on top of master. Submitting the fix in its own PR in to make sure I don't forget about it, in case my prerendering PR doesn't land as-is.

Prevents a push/pop mismatch when bailing out on HostRoots. This is
currently unobservable, because HostRoots never bail out on low
priority, but this does happen with prerendering.

I found this when rebasing facebook#10624 on top of master.
@@ -686,6 +686,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// TODO: Handle HostComponent tags here as well and call pushHostContext()?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this TODO btw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the same issue, but for HostComponents. This whole function is a code smell, IMO. Ideally we'd bail out on low priority before we ever call performUnitOfWork. I tried this out on my big refactor PR from earlier this summer. Will revisit when we add back resuming.

@acdlite acdlite merged commit b5ac963 into facebook:master Sep 14, 2017
@acdlite acdlite deleted the fixtoplevelhostcontextmismatch branch September 14, 2017 22:23
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.

3 participants