Skip to content
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

feat: Add filterNode option to prettyDOM #907

Merged
merged 13 commits into from
Jun 11, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 1, 2021

BREAKING CHANGE: <script />, <style /> and comment nodes are now ignored by default in prettyDOM .If you whish to return to the old behavior, use a custom filterNode function. In this case prettyDOM(element, { filterNode: () => true }).

I would find this change useful. But I believe this is possible to do in userland so I'm preparing a PR that just implements this in userland. We can then decide whether we want to defer to userland, release it as a breaking change or make it backwards compatible.

What:

Ignore <script />, <style /> and comment nodes in prettyDOM

Why:

test frameworks like karma and mocha add <script /> and comment nodes that add quite a bit of noise making debugging harder.

Would also improve #902

How:

Fork DOMElement plugin from pretty-dom to allow filtering single Node instances.

Checklist:

  • Documentation added to the
    docs site
    Will do when we release the stable v8
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@eps1lon eps1lon added enhancement New feature or request BREAKING CHANGE This change will require a major version bump labels Mar 1, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e949974:

Sandbox Source
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #907 (e949974) into alpha (532106b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             alpha      #907   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        25    +1     
  Lines          902       914   +12     
  Branches       279       286    +7     
=========================================
+ Hits           902       914   +12     
Flag Coverage Δ
node-10.14.2 100.00% <100.00%> (ø)
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-15 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/queries/text.ts 100.00% <ø> (ø)
src/suggestions.js 100.00% <ø> (ø)
src/config.ts 100.00% <100.00%> (ø)
src/pretty-dom.js 100.00% <100.00%> (ø)
src/shared.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532106b...e949974. Read the comment docs.

@eps1lon eps1lon changed the title Add expected behavior feat: Ignore <script /> and comment nodes when logging the DOM Mar 1, 2021
@eps1lon eps1lon changed the title feat: Ignore <script /> and comment nodes when logging the DOM BREAKING CHANGE: Ignore <script /> and comment nodes when logging the DOM Mar 1, 2021
@MatanBobi
Copy link
Member

I can see the value of this one but it is a breaking change and should be considered thoroughly.
What about adding this one behind a configuration?

@eps1lon
Copy link
Member Author

eps1lon commented Mar 7, 2021

I can see the value of this one but it is a breaking change and should be considered thoroughly.
What about adding this one behind a configuration?

I think I referred to all of this in the PR description: "We can then decide whether we want to defer to userland, release it as a breaking change or make it backwards compatible."

@nickserv
Copy link
Member

Perhaps we could hide this behind a config boolean, and set the default to true on a major release.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 18, 2021

I appreciate the review but let's focus on the high level problem: Do we want this as a feature or breaking change? What is the use case for the current behavior?

Then we can talk about the implementation. The implementation is problematic for a feature since it adds configuration but trivial to do.

@MatanBobi
Copy link
Member

I appreciate the review but let's focus on the high level problem: Do we want this as a feature or breaking change? What is the use case for the current behavior?

I'm actually not so sure I'm in favor of adding this one. I think that isn't a part of our responsibility and may just cause bugs/issues in some cases. As you already wrote, this can be patched on the user-land so I'm not sure a solution on our end is really necessary.
That's just my opinion of course.

@eps1lon
Copy link
Member Author

eps1lon commented Mar 24, 2021

I think that isn't a part of our responsibility and may just cause bugs/issues in some cases.

I think neither of these statements are relevant. What is our responsibility is better error diagnostics. So the question is if this helps or hinders error diagnostic which we should talk about.

Causing bugs is inherent to any change. The question is how likely they are.

As you already wrote, this can be patched on the user-land so I'm not sure a solution on our end is really necessary.

I believe. I haven't actually done it so let's not jump to conclusions and assume that the userland implementation is trivial.

@kentcdodds
Copy link
Member

We already have DEFAULT_IGNORE_TAGS:

export const DEFAULT_IGNORE_TAGS = 'script, style'

That's used for the ignore option on queries. I wonder whether it'd be possible to ignore everything that the failing query would ignore by default. That would be more correct anyway because right now we're displaying stuff that actually couldn't be queried.

@kentcdodds
Copy link
Member

To be clear, I'm in favor of this change in light of that :)

@eps1lon
Copy link
Member Author

eps1lon commented Apr 24, 2021

We already have DEFAULT_IGNORE_TAGS:

That makes a lot of sense. I was a bit suprised that I couldn't find existing implementation. Will try to see if I can leverage DEFAULT_IGNORE_TAGS usage instead.

@kentcdodds
Copy link
Member

To be clear, I think we should try to use whatever the ignore configuration is for the query that's failed. In most cases this will be the default, but it's an important distinction.

@kentcdodds
Copy link
Member

In fact, it may be useful to also display the ignore configuration as well so folks know that not everything is being displayed.

@eps1lon eps1lon changed the title BREAKING CHANGE: Ignore <script /> and comment nodes when logging the DOM BREAKING CHANGE: Ignore <script />, <style />, and comment nodes when logging the DOM Apr 25, 2021
@@ -0,0 +1 @@
export const DEFAULT_IGNORE_TAGS = 'script, style'
Copy link
Member Author

Choose a reason for hiding this comment

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

Breaks up a circular dep between config and prettyDOM

src/DOMElementFilter.ts Outdated Show resolved Hide resolved
@eps1lon eps1lon changed the base branch from main to alpha June 8, 2021 09:50
@eps1lon eps1lon changed the title BREAKING CHANGE: Ignore <script />, <style />, and comment nodes when logging the DOM feat: Add filterNode option to prettyDOM Jun 8, 2021
@eps1lon eps1lon marked this pull request as ready for review June 8, 2021 10:25
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks for this work!

Not excited about vendoring the DOMElementFilter file. I'm guessing they weren't receptive to a PR for this feature?

How much work would it be to take the ignore option into account for the errors that are thrown? For example, let's say I'm trying to find text within a style tag. I would set the selector and ignore values like so:

screen.getByText('.css-selector', {selector: 'style', ignore: 'script'})

Then my error should only show the things that could have matched. And it should show the selector and ignore configuration options as well.

So would it be possible to automatically filter the error message print out in a way that matches the selector and ignore values? I just think that a mismatch in those two could lead to confusing results.

I think it would amount to passing the selector and ignore options to prettyDOM and when those are provided, a filterNode function is created which only let's nodes that match the selector and don't match the ignore through. That way the error message displayed shows exactly what the user could have queried for given the options.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 8, 2021

Not excited about vendoring the DOMElementFilter file.

Me neither 😄

I'm guessing they weren't receptive to a PR for this feature?

It was pretty cumbersome to integrate while not breaking all the other tests. They generally weren't receptive to filtering stuff out at all which makes sense considering pretty-format is used for generating snapshot matchers. And if you want to filter stuff out from snapshots you're better off implementing custom snapshot matchers. But that's not how we're using pretty-format.

I think I'm generally fine with the "fork now, upstream later" approach here.

I just think that a mismatch in those two could lead to confusing results.

This is a real concern I agree. But we might want to revisit if we want to consider screen.getByText('.css-selector', {selector: 'style', ignore: 'script'}) valid in the first place. I can't imagine that this is a useful query. It looks like querying implementation details when you're better off using querySelect(.css-selector) and assert the computed styles of that element.
In other words: Is the effort worth the edge cases it covers or does this edge case virtually never happen i.e. we're aware of the defect and are willing to push a bug fix if somebody needs it.

@kentcdodds
Copy link
Member

I think I'm generally fine with the "fork now, upstream later" approach here.

Agreed.

valid in the first place.

Yeah, I was worried my contrived example would be distracting 😅

I think in general the biggest problem is the mismatch between what we see in the error and what we ignore/select. If you don't use ignore or select then it's fine because we can just go with the defaults, but once you use those options then it can become pretty unhelpful, and that's what I would like to avoid.

Is the effort worth the edge cases it covers or does this edge case virtually never happen i.e. we're aware of the defect and are willing to push a bug fix if somebody needs it.

That's fair. I currently don't have a use case for ignore or selector personally so I would be satisfied with a TODO code comment just so folks know that we'd like to support this in the future (rather than trying to figure out why we didn't do it in the first place).

@eps1lon
Copy link
Member Author

eps1lon commented Jun 8, 2021

I currently don't have a use case for ignore or selector personally so I would be satisfied with a TODO code comment just so folks know that we'd like to support this in the future (rather than trying to figure out why we didn't do it in the first place).

Sounds good. I just go ahead and add a test so that it's really "just" about fixing the problem. Makes it easier to get started contributing.

@eps1lon eps1lon requested a review from kentcdodds June 9, 2021 08:39
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

This would be a great addition to ATL, where I believe that Karma is the most popular. Just a small typo...

types/pretty-dom.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Solid 👍 Thanks!

@kentcdodds kentcdodds merged commit 9410e11 into testing-library:alpha Jun 11, 2021
@github-actions
Copy link

🎉 This PR is included in version 8.0.0-alpha.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump enhancement New feature or request released on @alpha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants