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

Issue with multiple children #9

Closed
grxy opened this issue May 23, 2017 · 8 comments
Closed

Issue with multiple children #9

grxy opened this issue May 23, 2017 · 8 comments

Comments

@grxy
Copy link
Contributor

grxy commented May 23, 2017

I ran into the following problem where not all nested children would be visited:

This works

const A = ({ children }) => children
const B = ({ children }) => <div>{children}</div>

const tree = (
    <A>
        <B />
        <B />
    </A>
)

reactTreeWalker(tree, visitor, context)

So does this

const A = ({ children }) => <div>{children}</div>
const B = ({ children }) => children

const tree = (
    <A>
        <B />
        <B />
    </A>
)

reactTreeWalker(tree, visitor, context)

This does not work

Because there are no basic HTML elements wrapping the children of A, those individual children are not visited

const A = ({ children }) => children
const B = ({ children }) => children

const tree = (
    <A>
        <B />
        <B />
    </A>
)

reactTreeWalker(tree, visitor, context)

A PR will follow shortly and should provide a fix for this issue.

@ctrlplusb
Copy link
Owner

Hey @grxy

Awesome, thanks so much for picking this up. I'll try review and merge within the next 24 hours. 👍

@grxy
Copy link
Contributor Author

grxy commented May 24, 2017

@ctrlplusb No prob, dude. I actually noticed the issue when writing unit tests for my code while using this.

P.S. Great work breaking it out from react-apollo!

@ctrlplusb
Copy link
Owner

Hi @grxy 👋

Apologies for the late one on this.

I have struggled to recreate your initial issue. I have added another branch containing a test that I was initially expecting to fail as it doesn't have your fix applied. The test passes though.

Do you mind having a look to see if I have configured the test correctly?

Thanks!

@grxy
Copy link
Contributor Author

grxy commented Jun 12, 2017

@ctrlplusb The issue happens when you have multiple react components that are siblings, so if you updated your tree to be:

const tree = (
      <OnlyChildren something={1}>
        <OnlyChildren something={2}>
          <div>Hello world!</div>
        </OnlyChildren>
        <OnlyChildren something={3}>
          <div>Hello world!</div>
        </OnlyChildren>
      </OnlyChildren>
    )

you would expect actual to be [1, 2, 3], when in fact the children are never visited, resulting in actual being only [1]. See screenshot:

image

@ctrlplusb
Copy link
Owner

Perfect thanks!

@grxy
Copy link
Contributor Author

grxy commented Jun 21, 2017

@ctrlplusb Any chance you'll be able to merge my PR and publish 2.1.2 soon?

ctrlplusb added a commit that referenced this issue Jun 21, 2017
Fix issue with nested children, update unit tests, close #9
@ctrlplusb
Copy link
Owner

All done and published. Thanks for being persistent with me!

@grxy
Copy link
Contributor Author

grxy commented Jun 21, 2017

@ctrlplusb Great! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants