Skip to content

Conversation

smacpherson64
Copy link
Collaborator

@smacpherson64 smacpherson64 commented Sep 3, 2018

What:
Extending matchers to allow the passing of NodeLists. (#52)

Why:
This allows users to not have to use loops over node lists when testing multiple elements.

How:
Created an HOF that wraps each matcher, if the first argument is a NodeList then it runs the matcher on all

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged
  • Added myself to contributors table

NOTE: This is the initial concept and may require tweaking, likely not ready to be merged.

@smacpherson64 smacpherson64 requested a review from gnapse September 3, 2018 13:05
@gnapse
Copy link
Member

gnapse commented Oct 12, 2018

I had forgotten about this. And to be honest, I'm very sorry to say that I'm not as convinced we need it, as I was before.

However, I'll take a look at how it behaves. I'm mostly worried about how will failing matchers convey the information of why it failed, given the potential of failing due to a lot of elements. That coped with the fact that this is merely a convenience for something users can do themselves relatively easily, makes me wonder.

I know we discussed this issue of conveying failed matches, and I'm sure you devoted significant effort in trying to make it usable. Just let me take a look at that, and at how this adds complexity to the source code. Hopefully, from your description above, this should not be something affecting the code of all matchers. This is something that would wrap any matcher meant for a single node, and inject the magic.

In the mean time, if you can help clarify some of these concerns, or argue why we need it, please do so.

@smacpherson64
Copy link
Collaborator Author

smacpherson64 commented Oct 12, 2018

Hey @gnapse,

I had forgotten about this. And to be honest, I'm very sorry to say that I'm not as convinced we need it, as I was before.

However, I'll take a look at how it behaves. I'm mostly worried about how will failing matchers convey the information of why it failed, given the potential of failing due to a lot of elements. That coped with the fact that this is merely a convenience for something users can do themselves relatively easily, makes me wonder.

Sounds good, even if we don't end up merging it in, it was fun to build! I think the display is pretty helpful and gives a little more context when an error occurs for a NodeList.

I know we discussed this issue of conveying failed matches, and I'm sure you devoted significant effort in trying to make it usable. Just let me take a look at that, and at how this adds complexity to the source code. Hopefully, from your description above, this should not be something affecting the code of all matchers. This is something that would wrap any matcher meant for a single node, and inject the magic.

Awesome, Thank you! please take a look! It does wrap the elements so the matchers themselves should not be affected.

In the mean time, if you can help clarify some of these concerns, or argue why we need it, please do so.

Makes sense, I will do my best to display a contrast:

The code itself for a simple example does not change significantly but is reduced for the end user. A more complex example, wanting to know how many elements failed or what the path is, would require a bit more on the end users part, but I assume the typical example would be like the following:

Before:

const elements = document.querySelectorAll('div');

for (element of elements.values()) {
    expect(element).someMatcher(somevalues);
}

After

const elements = document.querySelectorAll('div');
expect(elements).someMatcher(somevalues);

The message that a user would receive does change a bit, the before version stops at the first error. The after version checks all elements and displays more context:

  • How many elements failed / passed
  • Displays a sample of the elements and where they are located.

If a user is looping through elements in a NodeList and uses a selector that is too generic: 'div' currently they would need to dig or rely on the matcher to figure out what element the error occurred on. With the NodeList version it has the potential to improve that experience, because a path is displayed.

Before:

{matcher error}

After: (#52 (comment) is an example)

# of # elements failed

[index]: {shallow clone}
> path to element 

[index]: {shallow clone}
> path to element 

[index]: {shallow clone}
> path to element 

... (stops the sample at 3)

Error message for element [0]: 
============================
{matcher error}
============================

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.

2 participants