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

Make sure findWhere doesn't get stuck on empty string #1995

Merged
merged 1 commit into from Feb 2, 2019

Conversation

Projects
None yet
3 participants
@MikaelCarpenter
Copy link
Contributor

MikaelCarpenter commented Jan 31, 2019

What

  • Add a findWhere test case that replicates the use case I ran into
  • Update RSTTraversal.treeForEach to not call fn on empty strings
  • Update treeForEach test case that references empty strings

Why

In the event that the DOM has an empty string, make sure findWhere can iterate past it and still find elements that are further down the tree.

Links

@@ -40,7 +40,7 @@ export function hasClassName(node, className) {
}

export function treeForEach(tree, fn) {
if (tree !== null && tree !== false && typeof tree !== 'undefined') {
if (tree !== null && tree !== false && typeof tree !== 'undefined' && tree !== '') {

This comment has been minimized.

@MikaelCarpenter

MikaelCarpenter Jan 31, 2019

Author Contributor

would it make sense to just use if (tree) here, rather than blacklisting specific values?

This comment has been minimized.

@sstern6

sstern6 Feb 1, 2019

Contributor

Thanks for reporting and contributing. I think this is a good start. Though as newer versions of react come out, more and more valid "empty" values become natively supported by react. Ie emptyString, emptyArray, emptyFragments, undefined. Adding a fix for just an empty string seems like a good start and solves your problem but i think we need to consider other use cases including the ones i mentioned above.

@ljharb thoughts on this?

This comment has been minimized.

@ljharb

ljharb Feb 1, 2019

Member

i mean, realistically here, this is deciding when to bypass calling childrenOfNode.

A node can only have children if it's an object, right? Could this change be effectively if (isPrimitive(tree)) {? (with function isPrimitive(val) { return !val || Object(val) !== val; })

This comment has been minimized.

@MikaelCarpenter

MikaelCarpenter Feb 1, 2019

Author Contributor

So I tried this out and it looks like calling fn(tree) on things like strings and numbers is a desired functionality. Here are some of the tests that fail (trimmed out some of the output):

1) RSTTraversal treeForEach does not get trapped from empty strings:

2) (uses jsdom) mount .findWhere(predicate) does not pass in null or false nodes:

    AssertionError: expected [ Array(3) ] to deeply equal [ Array(4) ]
    + expected - actual

        [
          "<div>\n  foo bar\n</div>"
          true
        ]
    +  [
    +    "foo bar"
    +    [null]
    +  ]
      ]
    
    at Context.<anonymous> (packages/enzyme-test-suite/test/ReactWrapper-spec.jsx:1880:30)

3) (uses jsdom) mount .findWhere(predicate) allows `.text()` to be called on text nodes:

4) shallow .contains(node) works with strings:

5) shallow .contains(node) works with numbers:

6) shallow .contains(node) works with nested strings & numbers:

7) shallow .findWhere(predicate) allows `.text()` to be called on text nodes:

    AssertionError: expected [ Array(3) ] to deeply equal [ Array(4) ]
    + expected - actual

        [
          "<div>\n  foo bar\n</div>"
          "foo bar"
        ]
    +  [
    +    "foo bar"
    +    "foo bar"
    +  ]
      ]
    
    at Context.<anonymous> (packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx:1829:31)

8) selectors (uses jsdom) mount :empty pseudo selector:

9) selectors shallow :empty pseudo selector:

    AssertionError: expected {} to have a length of 0 but got 1
    + expected - actual

    -1
    +0
    
    at Context.<anonymous> (packages/enzyme-test-suite/test/selector-spec.jsx:371:52)

This comment has been minimized.

@MikaelCarpenter

MikaelCarpenter Feb 1, 2019

Author Contributor

So it looks like we only want to prevent falsey values. I tried out if (tree) { fn(tree) } and the tests will still pass.

This comment has been minimized.

@ljharb

ljharb Feb 1, 2019

Member

ah, ok - let's do that then :-)

@MikaelCarpenter MikaelCarpenter changed the title ISSUE-1994 Make sure findWhere doesn't get stuck on empty string Make sure findWhere doesn't get stuck on empty string Jan 31, 2019

@@ -40,7 +40,7 @@ export function hasClassName(node, className) {
}

export function treeForEach(tree, fn) {
if (tree !== null && tree !== false && typeof tree !== 'undefined') {
if (tree !== null && tree !== false && typeof tree !== 'undefined' && tree !== '') {

This comment has been minimized.

@ljharb

ljharb Feb 1, 2019

Member

i mean, realistically here, this is deciding when to bypass calling childrenOfNode.

A node can only have children if it's an object, right? Could this change be effectively if (isPrimitive(tree)) {? (with function isPrimitive(val) { return !val || Object(val) !== val; })

Show resolved Hide resolved packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated

@ljharb ljharb force-pushed the MikaelCarpenter:b/ISSUE-1994-empty-string-findWhere branch from 5f9e696 to ec3b89e Feb 2, 2019

@ljharb

ljharb approved these changes Feb 2, 2019

@ljharb ljharb merged commit ec3b89e into airbnb:master Feb 2, 2019

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ljharb ljharb removed the Shallow label Feb 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment