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

Add traversal for Fiber test renderer #10377

Merged
merged 1 commit into from
Aug 7, 2017

Conversation

sophiebits
Copy link
Collaborator

Not clear the path to shipping this but this gives us a migration path internally that we need right now (replaces https://fburl.com/udq9ksvk).

@sophiebits
Copy link
Collaborator Author

sophiebits commented Aug 4, 2017

@bvaughn Do you want to try to pull this in and see if our tests all pass (or tell me how to do it :p)? I wrote this tonight and the tests (copied from internal https://fburl.com/3vdfoyz7) all pass so it seems too good to be true but maybe.

@bvaughn
Copy link
Contributor

bvaughn commented Aug 4, 2017

Nice! Yes, I'll run this tomorrow morning.

@bvaughn
Copy link
Contributor

bvaughn commented Aug 4, 2017

Let's chat when you get in Ben!

@bvaughn
Copy link
Contributor

bvaughn commented Aug 7, 2017

Looks like CI is failing b'c of Prettier and some Flow issues

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks great Ben!

Aside from the Flow/Prettier issues, LGTM.

By the way, did you intend to add a small test for fragments?

predicate: Predicate,
options: ?FindOptions,
): Array<ReactTestInstance> {
const deep = options ? options.deep : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The name of this option threw me a bit. To me, "deep" relates to the location within the tree but seems more like returnFirst?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea it's a little subtle. It's "if a component and its descendants both match, do we return both or just the parent?" You'd think you'd always want all of them but spreading props is common enough that the pattern is painful otherwise unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not how I'm understanding the code. Isn't it a choice between:

  • If a component matches, return it, otherwise return the first descendant that matches.
  • Return the component and any of its descendants that match.

I think the thing that I find confusing is that deep:false can still return a matching descendant (if the component doesn't match).

Am I misunderstanding this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A
  B {testId: 'foo'}
    div {testId: 'foo'}
  section
    C {testId: 'foo'}
      span {testId: 'foo'}

Finding by props {testId: 'foo'} returns B and C if deep is false but returns B, C, div, and span if deep is true.

Anyway, I didn't write this code – copy/pasted this bit from the internal prototype of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I understand. 👍

Not clear the path to shipping this but this gives us a migration path internally that we need right now (replaces https://fburl.com/udq9ksvk).
@sophiebits sophiebits merged commit a6e20d0 into facebook:master Aug 7, 2017
@bvaughn bvaughn mentioned this pull request Aug 8, 2017
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