Skip to content

[Fiber] Fix TestUtils edge cases #8257

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

Merged
merged 1 commit into from
Nov 10, 2016
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 10, 2016

  • Fixes an issue where functional components were ignored when "scrying"
  • Fixes an issue where we looked at work in progress instead of the current node

The rest of TestUtils failures are now all Simulate related.

while (root.return) {
root = root.return;
}
var isRootCurrent = root.tag === HostContainer && root.stateNode.current === 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.

I'm not sure if this is a good way to tell that we're in a current tree. Any pitfalls?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm facing the same problem in the PR I'm working on.

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

👍 but let's decide (or clarify) the established way to determine whether a fiber is current

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

Hmm, I don't think following the return path guarantees that a fiber is current. They aren't parallel trees: #7344 (comment)

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 10, 2016

What's the right way to get the current version of a given Fiber? I tried looking at ReactFiberReflection but it searches for host parents whereas I want to search for the current version of this fiber.

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

I think isFiberMounted is what you want. It searches for host parents because if a fiber is current then it follows that it has a HostContainer parent.

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

So maybe

function getCurrentFiber(fiber : Fiber): Fiber | null {
  if (isFiberMounted(fiber)) {
    return fiber;
  } else if (fiber.alternate && isFiberMounted(fiber.alternate)) {
    return fiber.alternate;
  } else {
    return null;
  }
}

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 10, 2016

I tried this but isFiberMounted is true for both of them.

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

Huh interesting. Maybe what you did is correct then. Can't say I fully understand, though.

Could you put your solution into ReactFiberTreeReflection? I need it in my PR as well. If it's wrong in some way we can fix it later.

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

Actually I think I understand now. Your solution is correct 👍

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 10, 2016

Haha now I'm not sure if it's correct. 😄

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 10, 2016

I think #7344 (comment) doesn't apply because I don't care if either node on the path is current or not. I only care about the root node for which I can find it out. I also care that if a node's root is current, it follows that the node is current. Does this assumption always hold?

@acdlite
Copy link
Collaborator

acdlite commented Nov 10, 2016

I think it does because the return field is always set during reconciliation in ReactChildFiber

* Add more reactComponentExpect tests

* Implement reactComponentExpect in Fiber
@gaearon gaearon merged commit a9fa135 into facebook:master Nov 10, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Nov 10, 2016

OK, let's do it and if it's broken then add a breaking test.

@gaearon gaearon deleted the test-utils-fiber branch November 10, 2016 21:27
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
* Add more reactComponentExpect tests

* Implement reactComponentExpect in Fiber
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.

4 participants