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

Fix Suspense-wrapping heuristic #19368

Closed

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 15, 2020

Fixes #18831

This PR also includes a package version bump (16.13.1 -> 17.0.0-alpha.0) so that DevTools tests will pass CI.

PR #19108 broke DevTools Suspense integration.

DevTools uses the renderer's version number to figure out what the tag numbers are:

if (gte(version, '16.6.0-beta.0')) {
ReactTypeOfWork = {
Block: 22,
ClassComponent: 1,
ContextConsumer: 9,
ContextProvider: 10,
CoroutineComponent: -1, // Removed
CoroutineHandlerPhase: -1, // Removed
DehydratedSuspenseComponent: 18, // Behind a flag
ForwardRef: 11,
Fragment: 7,
FunctionComponent: 0,
HostComponent: 5,
HostPortal: 4,
HostRoot: 3,
HostText: 6,
IncompleteClassComponent: 17,
IndeterminateComponent: 2,
LazyComponent: 16,
MemoComponent: 14,
Mode: 8,
OffscreenComponent: 23, // Experimental
Profiler: 12,
SimpleMemoComponent: 15,
SuspenseComponent: 13,
SuspenseListComponent: 19, // Experimental
YieldComponent: -1, // Removed
};
} else if (gte(version, '16.4.3-alpha')) {
ReactTypeOfWork = {
Block: -1, // Doesn't exist yet
ClassComponent: 2,
ContextConsumer: 11,
ContextProvider: 12,
CoroutineComponent: -1, // Removed
CoroutineHandlerPhase: -1, // Removed
DehydratedSuspenseComponent: -1, // Doesn't exist yet
ForwardRef: 13,
Fragment: 9,
FunctionComponent: 0,
HostComponent: 7,
HostPortal: 6,
HostRoot: 5,
HostText: 8,
IncompleteClassComponent: -1, // Doesn't exist yet
IndeterminateComponent: 4,
LazyComponent: -1, // Doesn't exist yet
MemoComponent: -1, // Doesn't exist yet
Mode: 10,
OffscreenComponent: -1, // Experimental
Profiler: 15,
SimpleMemoComponent: -1, // Doesn't exist yet
SuspenseComponent: 16,
SuspenseListComponent: -1, // Doesn't exist yet
YieldComponent: -1, // Removed
};
} else {
ReactTypeOfWork = {
Block: -1, // Doesn't exist yet
ClassComponent: 2,
ContextConsumer: 12,
ContextProvider: 13,
CoroutineComponent: 7,
CoroutineHandlerPhase: 8,
DehydratedSuspenseComponent: -1, // Doesn't exist yet
ForwardRef: 14,
Fragment: 10,
FunctionComponent: 1,
HostComponent: 5,
HostPortal: 4,
HostRoot: 3,
HostText: 6,
IncompleteClassComponent: -1, // Doesn't exist yet
IndeterminateComponent: 0,
LazyComponent: -1, // Doesn't exist yet
MemoComponent: -1, // Doesn't exist yet
Mode: 11,
OffscreenComponent: -1, // Experimental
Profiler: 15,
SimpleMemoComponent: -1, // Doesn't exist yet
SuspenseComponent: 16,
SuspenseListComponent: -1, // Doesn't exist yet
YieldComponent: 9,
};
}

As of #19108, DevTools also decides how to traverse Suspense fibers using the tag values as a form of feature detection:

const areSuspenseChildrenConditionallyWrapped =
OffscreenComponent === -1;
if (areSuspenseChildrenConditionallyWrapped) {
primaryChild = fiber.child;
} else if (fiber.child !== null) {
primaryChild = fiber.child.child;
}

The reason this PR also bumps version numbers is that we previously had multiple Suspense implementations published under 16.13.1 (both the NPM releases and our CI-built releases that tests run against), so the DevTools heuristic failed.

To prevent this type of regression from happening again, a larger effort needs to be made in the form of follow up PRs to get CI running DevTools tests against a range of React releases (not just what's in master).

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 15, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 43d92f8:

Sandbox Source
React Configuration

@@ -716,4 +716,93 @@ describe('ProfilingCache', () => {
TestRenderer.create(<Validator commitIndex={0} rootID={rootID} />);
});
});

// See https://github.com/facebook/react/issues/18831
it('should not crash during route transitions with Suspense', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This newly added test caught the regression between 16.13.1 (NPM release) and previous DevTools master.

@sizebot
Copy link

sizebot commented Jul 15, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 43d92f8

@sizebot
Copy link

sizebot commented Jul 15, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 43d92f8

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2020

To be clear, I think we should still move forward with this fix and release an updated version of DevTools since this fixes the issue all open source users (who use Suspense) are currently facing. This can be confirmed by building and running the extension against this repro case I created: https://cx4rd.csb.app/

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2020

CI is failing because of the shallow renderer, which no longer lives in this repo. We can't update to point at v17 of it because that doesn't exist, and the latest version the does exist references react-is v16.

@bvaughn bvaughn force-pushed the devtools-suspense-heuristic-fix branch from 0025608 to 972461a Compare July 15, 2020 14:52
@bvaughn bvaughn closed this Jul 15, 2020
@bvaughn bvaughn reopened this Jul 15, 2020
@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2020

I'm going to try removing all references to the shallow renderer to see if this fixes our CI failures. We can talk more before committing to this (or we could move forward and revert that specific part of the commit before the actual v17 release) but it would be nice to buy ourselves time without having to commit to v17 of shallow renderer.

@bvaughn bvaughn reopened this Jul 15, 2020
@bvaughn bvaughn closed this Jul 15, 2020
@bvaughn bvaughn reopened this Jul 15, 2020
@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 15, 2020

This PR seems to have gotten screwed up by a Circle CI issue, so I'm just going to close it and move to #19373

@bvaughn bvaughn closed this Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: "Commit tree does not contain fiber 256. This is a bug in React DevTools."
5 participants