Fix descendant selector

[ReactiveRaven]

Given a node like:

<div className="test">
    <div></div>
</div>

a selector like .find(".test div") or .find("div div") unexpectedly matches the outer div, as well as the inner div.

The patch uses child nodes of current matches, which excludes the 'parent' match.

[ljharb]

This is intentional; if you want to restrict to the children, you'd do .children().find(…)

[ljharb]

this would be a breaking change.

[ReactiveRaven]

@ljharb Ah, I see. I'm glad for the workaround, thanks!

...But, this doesn't match CSS behaviour because the outer div has no ancestor that matches.

elements matched by the second selector are selected if they have an ancestor element matching the first

Is this a 'breaking change' meaning:

• do-not-accept; docs need updating to clarify descendant selectors do not work as expected, or
• accept-later; but it will need to be rolled into a version bump?

the docs currently just say:

enzyme also gives support for the following contextual [css] selectors .foo .bar .foo input

Thanks.

[ljharb]

It already doesn't match CSS behavior because there's both host nodes and Component instances in the tree :-)

It's a breaking change meaning, I don't think it's worth accepting it at this time - and I don't think it'd be an improvement.

As for .find(".test div"), can you share your actual component and test code?

[ReactiveRaven]

Ah, I see. I'll close this PR and open one to update the docs then.

The component was mapping pseudo-markdown so it could be styled easier:

export default class Section extends React.Component {
    renderLine(line, index) {
        switch (line.slice(0, 2)) {
            case "# ":
                return <div key={`${index}`} className={"heading"}>{line.slice(2)}</div>
            default:
                return <div key={`${index}`} className={"paragraph"}>{line}</div>
        }
    }

    render() {
        return (
            <div className={"section"}>
                {this.props.lines.map(this.renderLine)}
            </div>
        )
    }
}

The test was:

it("should display the expected number of results", () => {
    const LINES = [ "# Hello world", "Hello world" ]
    const component = shallow(<Section lines={LINES} />)
    expect(component.find(".section div").length).toEqual(LINES.length)
})

which I expected to return the inner divs generated by the .map(, but also returned the outer div.

(I could have used .children().length, but I had been successfully using .find("foo bar") elsewhere in the tests so I used it again for consistency. Now I know it is broken, I'll stop using them elsewhere as well, and use chaining in stead)

[ReactiveRaven]

As discussed in #1700 , there is a bug here, but it needs updating to handle both host-nodes and components first. Next step is a test case to define that behaviour.

[ljharb]

@ReactiveRaven any update on updating the test cases?

[ReactiveRaven]

(and now from the correct account)

@ljharb Hey sorry, just realised I posted it in #1700 but didn't make it clear here:

If you can describe/provide a test case for the intended behaviour when working with non-host-nodes, I'll happily get that PR fixed up.

So, I need from you an example situation that you are concerned about this change breaking. Then I can turn that into the test case and make sure nothing breaks.

[ljharb]

ahhh thanks for clarifying. In this case, I suspect if you rebase your branch, existing tests will fail if it's indeed breaking.

[ReactiveRaven]

@ljharb Rebase completed! Test & lint passes locally; the two failures on travis were:

• a timeout with no explanation 🙁
• complaining of a missing TS definition in lodash (flatten, which was already used in that file before my changes)

So I triggered a rerun, and got:

• a timeout with no explanation, but on a different one this time (mysterious!)
• the same lodash complaint.

So I rm -rf node_modules; npm installed and tried again, and it looks like some dev-dependencies were removed from the package.json; I found react and react-dom weren't installed, which caused npm run lint to complain. Once I ran npm install --save-dev react react-dom, it works again locally. It seems like something odd is going on with dependencies, maybe that is why it is flagging flatten?

Can you shed any light?

I was rebasing off origin/master not upstream/master 😨. Trying again.

[ReactiveRaven]

Good news: Now the only failure is a timeout! 🎉 Using the right remote really helps.

Bad news: I'm worried the test command might not be running all the tests? The last successful push shows only three tests, and <25% branch coverage - locally, I tried editing a test to deliberately make it fail (lengthOf(3) -> lengthOf(3000)) and the tests still passed.

I took a look, and it looks like a describe.only has snuck in!

[ReactiveRaven]

Looks like this has already been reported in #1741. When I manually apply the fix from that PR, all tests pass, so I guess this PR is blocked on #1741 for now.

[ljharb]

That's now merged, and I've rebased this one.

[ghost]

@ljharb Thanks! Looks like you have better luck with timeouts, we have a green tick :)
Did you still want to add an extra test to be super sure it works right?

[ljharb]

Yes, I think an additional test that deals with custom components would be helpful.

[ReactiveRaven]

OK sure. Can you describe what that test should look like?

[ljharb]

@ReactiveRaven i think just something that's not selecting on just html - ie like div Foo span

[ljharb]

Thanks; I've tweaked the test a bit; will merge once it passes.

[ReactiveRaven]

Thanks 😄 🎉

[tvararu]

Hello! 👋

We had some tests that were broken by this change. I can see why this was packaged as a patch release, but it is functionally a breaking change.

I think ideally the way this should have been handled is:

• written passing tests for this unintended behaviour
• documented as an unintended behaviour
• delayed the API change to 4.0

However it's not a big deal.

[ljharb]

@tvararu if the standard for “breaking” is “made any previously passing tests fail” then a huge class of bugs wouldn’t be able to be fixed. In this case, the tests that failed never should have passed in the first place, and it was a bug that they did.

[tvararu]

@ljharb I want to avoid a debate about the meaning of SEMVER and bugs vs features in software.

The fact is that the API changed in a way that is not backwards compatible. It doesn't matter if this was to address an existing bug / an incorrectly documented API; it just is.

For what it's worth, jQuery did the same thing by doing a patch release for a backwards incompatible bug fix in one of its functions. That function was being used in that specific incorrect way in the jquery-tabs library, and it ended up breaking the GOV.UK bank holiday page. There's a relevant GitHub issue thread about this on the jQuery project but I can't find it right now.

Those are my full thoughts on this issue; I believe that in perspective the impact of this change is quite a lot smaller, and I'm not looking for someone else to blame for why our builds were broken for 2 hours. I was simply suggesting how I think this would have been handled if SEMVER was indeed followed to the letter.

All the best. :)

[ljharb]

It definitely matters; because you’re invoking the meaning of semver if you’re claiming it’s a breaking change.

Semver is very fuzzy, it does not have a “letter” here - and if it did, the documented api of our selectors was broken, and this fixes it, making it objectively a patch. That it broke your build is actually irrelevant to semver.

[ReactiveRaven]

@tvararu Hi! While I agree it sucks that your tests started failing, according to SemVer, this change actually is 'backwards compatible', because it behaves the way the documentation says it should. SemVer refers only to the 'public API', which for Enzyme is defined in the documentation (semver 2.0.0 item 1). The implementation is distinct from the public API, which allows for 'bug fixes' to update incorrect behavior in the implementation (item 6) as long as it does not change the public API. It is not a major change, because the public API has not changed (item 8). In this case, if your project broke as a result of this change, it was depending on an internal implementation detail (in this case, a 🐛, like ljharb says), not the public API as defined by Enzyme's documentation.

The easiest way to avoid this in future would be to note when a feature of a project diverges from the stated public API, immediately raise an issue/ticket (or subscribe to/vote on existing ones), then not use that feature in your project so you avoid writing code that depends on a bug.

Anyway. Thank you for the excuse to go refresh myself on SemVer, and congrats on the new, more correct, test suite. All the best :)

[tvararu]

That it broke your build is actually irrelevant to semver.

@ljharb this is true, but nobody likes having their stuff broken, regardless of the issue.

@ReactiveRaven your explanation makes perfect sense. Thank you!

The easiest way to avoid this in future would be to note when a feature of a project diverges from the stated public API, immediately raise an issue/ticket (or subscribe to/vote on existing ones), then not use that feature in your project so you avoid writing code that depends on a bug.

I agree. The reason this didn't happen in this particular situation was that the original code actually contained a typo:

// What was written
component.find('.class .another-class');

// What was intended
component.find('.class.another-class');

So we didn't realise we were relying on a bug. Went unnoticed for months before this.

I take back my comment about "SEMVER being followed to the letter."

Genuinely all the best and carry on the good open sourcing. 👌

[ljharb]

To be quite honest, we had one or two tests inside @airbnb that suffered from that exact same bug, that this update helped us catch 😌