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

Alternate fix for "Fix a crash in Suspense with findDOMNode" #15312

Merged
merged 2 commits into from Apr 3, 2019

Conversation

@acdlite
Copy link
Member

commented Apr 3, 2019

This doesn't rely on checking the tag. When the alternate of a parent is missing, it assumes it's a fragment indirection and moves onto the next parent fiber.

I based my commit on top of @gaearon's PR #15195.

@acdlite acdlite requested a review from gaearon Apr 3, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Note to self: Assuming we land this, make sure we sync it to the new DevTools package too https://github.com/bvaughn/react-devtools-experimental/blob/07bf8e53c1e6fbf6503bebc170622ab161cf3b8f/src/backend/renderer.js#L1105-L1106

@sizebot

This comment has been minimized.

Copy link

commented Apr 3, 2019

Fails
🚫

node` failed.

Log

Error:  { FetchError: invalid json response body at http://react.zpao.com/builds/master/_commits/43b1f74c88d986c88623412be7b1d65a6e271779/results.json reason: Unexpected token < in JSON at position 0
    at /home/circleci/project/node_modules/node-fetch/lib/body.js:48:31
    at process._tickCallback (internal/process/next_tick.js:68:7)
  name: 'FetchError',
  message:
   'invalid json response body at http://react.zpao.com/builds/master/_commits/43b1f74c88d986c88623412be7b1d65a6e271779/results.json reason: Unexpected token < in JSON at position 0',
  type: 'invalid-json' }

Generated by 🚫 dangerJS

@sebmarkbage
Copy link
Member

left a comment

This only deals with one of the paths hitting this case.

If we're traversing backwards in the inverse case, then we'll skip past the extra fragment on one of the nodes. It seems like something else can go wrong in that scenario.

@acdlite

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Oops forgot to run Flow

@@ -117,11 +117,19 @@ export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null {
let b = alternate;
while (true) {
let parentA = a.return;

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Apr 3, 2019

Member

What if parentA here is a Suspense boundary with no fragment and its alternate is a Suspense boundary with a fragment?

In that case the children won't be representing the same thing. However, they also will never be equal so maybe it doesn't matter?

This comment has been minimized.

Copy link
@acdlite

acdlite Apr 3, 2019

Author Member

That was my thinking, at least

@acdlite

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I'll squash my commits (but leave @gaearon's separate) and land via command line so @gaearon also gets credit

Alternate fix for #14198
This doesn't rely on checking the tag. When the alternate of a parent
is missing, it assumes it's a fragment indirection and moves onto the
next parent fiber.

@acdlite acdlite force-pushed the acdlite:alternate-fix-for-14198 branch from 982252d to 43b1f74 Apr 3, 2019

@acdlite acdlite merged commit 43b1f74 into facebook:master Apr 3, 2019

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Thanks folks!

Synced to DevTools via bvaughn/react-devtools-experimental@7fbdfd9

@gaearon gaearon referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.