Skip to content

Conversation

@ljharb
Copy link
Member

@ljharb ljharb commented Feb 3, 2016

…er#props()` should work with `null` nodes.

Fixes #113.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think in the Utils file there is a propsOfNode function that we should probably be using instead of this. It deals with things like null being passed in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good - will that work the same in shallow and mount?

@ljharb ljharb force-pushed the shallow_fix_nulls_in_sfcs branch from c7d0670 to 198db6c Compare February 3, 2016 05:45
lelandrichardson added a commit that referenced this pull request Feb 4, 2016
[Fix] `{ShallowWrapper,ReactWrapper}#{type,props}()` should work with `null` nodes
@lelandrichardson lelandrichardson merged commit 3c06ba1 into master Feb 4, 2016
@lelandrichardson lelandrichardson deleted the shallow_fix_nulls_in_sfcs branch February 4, 2016 01:11
@jedwards1211
Copy link

@ljharb I'm wondering -- why would we want wrappers with null nodes to get passed to findWhere anyway? I'm assuming there are still other methods that won't work with null nodes (I stumbled on this problem with .text()). Wouldn't it be more robust to simply not pass wrappers with null nodes?

@ljharb
Copy link
Member Author

ljharb commented Feb 4, 2016

That's a good point, and may be an alternative approach.

@jedwards1211
Copy link

Yeah I was going to post an issue about .text() in .findWhere() but I could just post an issue about null nodes instead

@ljharb
Copy link
Member Author

ljharb commented Feb 4, 2016

Let's do that - findWhere and filterWhere etc perhaps shouldn't return null nodes? It's worth a discussion.

@jedwards1211
Copy link

Okay, sounds good

@jedwards1211
Copy link

And thanks for this awesome project!

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