Skip to content

Conversation

@thien-do
Copy link
Contributor

Fixes #19

@codecov
Copy link

codecov bot commented Mar 11, 2018

Codecov Report

Merging #23 into master will increase coverage by 0.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #23      +/-   ##
=========================================
+ Coverage   85.91%   86.3%   +0.38%     
=========================================
  Files           1       1              
  Lines          71      73       +2     
  Branches       20      21       +1     
=========================================
+ Hits           61      63       +2     
  Misses          8       8              
  Partials        2       2
Impacted Files Coverage Δ
src/index.js 86.3% <100%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5951226...4425daf. Read the comment docs.

src/index.js Outdated
// - https://github.com/ctrlplusb/react-tree-walker/issues/19
// - https://github.com/gaearon/react-hot-loader/issues/832
// - https://github.com/gaearon/react-hot-loader/blob/master/src/proxy/utils.js#L84
const theChild = child && typeof child.render === 'function'

Choose a reason for hiding this comment

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

Could one intermediate component return another?
Maybe this should be recursive.

Copy link
Contributor Author

@thien-do thien-do Mar 11, 2018

Choose a reason for hiding this comment

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

Do you mean something like this:

// as a helper at root level:
const flattenChild = component => (
  component && typeof component.render === 'function'
    ? flattenChild(component.render())
    : component
);

// then inside doTraverse function:
const child = getChildren();
const theChild = flattenChild(child);

or

// inside doTraverse function:
let child = getChildren();
while (child && typeof child.render === 'function') {
  child = child.render();
}

I'm not sure which one is better here yet

Choose a reason for hiding this comment

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

I'll prefer the first one. It is just testable, and look like code coverage is a thing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update shortly. Meanwhile, if you have time, can you help me on having some tests for this? Like how to have a sample child with render... Maybe something like this?

const Foo = () => (
  () => <span>abc</span>
);

Choose a reason for hiding this comment

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

class MyComponent extends Component { render() { return <div /> }}

const Foo = (props) => new MyComponent(props);

@theKashey
Copy link

So - in case of "instance" in tree - just "skip" it by rendering, and dive deeper.
Should work, but I'll and a test for this case. Just to be sure.

@thien-do
Copy link
Contributor Author

thien-do commented Mar 11, 2018

@theKashey I added a test and add support for recursive calling render until we have a valid React child.

Copy link
Contributor

@birkir birkir left a comment

Choose a reason for hiding this comment

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

Looks good, can you merge and release @ctrlplusb
This is necessary for react-universally @ next

@ctrlplusb ctrlplusb merged commit 8d8c74b into ctrlplusb:master Mar 14, 2018
@thien-do thien-do deleted the fix-instance-as-result branch March 14, 2018 15:05
@ctrlplusb
Copy link
Owner

Thanks so much for this @dvkndn! And for @theKashey and @birkir for the review 👍

daohoangson added a commit to daohoangson/js-tinhte-api that referenced this pull request Mar 14, 2018
ctrlplusb added a commit that referenced this pull request Mar 20, 2018
Support instance-as-result children
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

Successfully merging this pull request may close these issues.

4 participants