Use Query instead of Selector

[tesk9]

Updates Query to look at className and class properties/attributes, rather than relying only on the classNames.

Some context can be found in elm-explorations#175 (review)

[Confidenceman02]

I both love this, and have questions.

❤️ I love how clever and cleaner it is to my implementation. Awesome job!

🤔 We are touching how hasClass works and this might break peoples tests in weird and wonderful ways. Might using a more verbose svg specific abstraction that leaves current code paths intact be safer?

🤔 hasExactClassName will mysteriously not work for svgs but work for html. Might that cause me confusion and make it difficult to reason about my test considering hasClass and hasClasses work fine for both html and svg?

[rtfeldman]

this might break peoples tests

Hm, I don't think this could break any existing tests, could it? 🤔

It's doing a List.member on a strictly larger list, so is there any way it could fail when previously it succeeded?

Edit: hasNot would do it. So yeah, this could cause regressions in tests.

What would such a regression look like though?

    |> Query.hasNot [ class "blah" ]

So in order for this to cause a regression, the broken test would have to:

• Include a Query.hasNot using a class selector
• Have it be for an element that has that exact class, but it's set as a class property when it should be an attribute (or vice versa - I forget which, but you get the idea)

To me that sounds more likely to expose an implementation bug (e.g. they thought the test was covering this situation but actually it wasn't because they were using an attribute when they should have used a property in the implementation) than to break a valuable test!

[rtfeldman]

Also, what is hasExactClassName? 😅

Edit: oh - did you mean exactClassName?

[Confidenceman02]

[ test "does not find class on svg elements" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ]
                    [ Svg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ]
                    |> Query.fromHtml
                    |> Query.hasNot [ class svgClass ]

This is a test I wrote to assert that the svg class was not being found. With these changes that would break.

Despite how useful we think this test is, the fact is, the changes would break previously passing tests.

Also, what is hasExactClassName? 😅

I meant exactClassName 😅

exactClassName is an existing function that does not split the class names. So it will assert the entire class string. Here is NS equivalent I wrote to mimic the behaviour.

test "exactClassNameNS selector finds the exact class value on svg" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class another-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ]
                    [ Svg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ]
                    |> Query.fromHtml
                    |> Query.has [ exactClassNameNS svgClass ]

[rtfeldman]

This is a test I wrote to assert that the svg class was not being found. With these changes that would break.

Right - certainly it can break, but I think it's relevant to consider the likelihood that this actually comes up.

Can we think of a real-world example where all of the following are true?

1. Someone wrote code using hasNot with class
2. That query applies to a node which has that particular class specified as a property, not an attribute (so the test passes today but would break with this change)
3. If the test failed, they would be upset because 1. and 2. are exactly the behavior they wanted to get out of the test - as opposed to being happy because the test failing led them to realize that the test was actually passing by accident (because they had specified a property, not an attribute, unintentionally - meaning the browser saw the class but the selector didn't, at least the way the selector works today) and leading them to change their code for the better.

Personally I'm finding it hard to imagine how all 3 of those could be true in someone's code base today, but I could be missing something! 😄

[Confidenceman02]

Im certainly not going to make assumptions about how people test their code, just raising the point that we are touching a path that changes test behaviour on nodes other than svg, which was the focus of the fix. Whether this has other weird and wonderful side effects, not sure?

It seems you are favouring this approach? I am happy to support yours and @tesk9 's intuition on this.

@tesk9 Would you be keen to implement the exactClassName implementation?

[tesk9]

If we're sure we want exactClassName to match both attributes and properties, I'm happy to implement!