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

Support instance-as-result children #23

Merged
merged 1 commit into from
Mar 14, 2018

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