feat(custom normalizer): allow custom control of normalization #172

RoystonS · 2018-12-08T17:09:12Z

What:

(PR for issue #171)

Adds an optional {normalizer} option to query functions; this
is a transformation function run over candidate match text
after it has had trim or collapseWhitespace run on it,
but before any matching text/function/regexp is tested
against it.

The use case is for tidying up DOM text (which may
contain, for instance, invisible Unicode control characters)
before running matching logic, keeping the matching logic
and normalization logic separate.

Why:

It's otherwise not easy to apply normalization logic to DOM text without having to replace large portions of the matching logic.

How:

Added a new optional normalizer function which applies after trim + collapseWhitespace but before matching logic.

I had to make the same changes in several places in the code as there's a lot of similar option-handling code around, unfortunately. That also meant lots of test cases to ensure all those paths are tested.

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

Adds an optional {normalizer} option to query functions; this is a transformation function run over candidate match text after it has had `trim` or `collapseWhitespace` run on it, but before any matching text/function/regexp is tested against it. The use case is for tidying up DOM text (which may contain, for instance, invisible Unicode control characters) before running matching logic, keeping the matching logic and normalization logic separate.

alexkrolick · 2018-12-08T19:33:29Z

src/__tests__/text-matchers.js

@@ -194,3 +194,115 @@ cases(
    },
  },
 )
+
+// A good use case for a custom normalizer is stripping
+// out UCC codes such as LRM before matching


Can we expand out acronyms like this for those who might be unfamiliar with Unicode (UCC -> Unicode Control Character)? 😄

Yep, I did expand it out in the user-facing docs, but I'll expand it here too.

kentcdodds

Thanks! I have some thoughts around this.

src/matches.js

This commit moves the implementation of `trim` + `collapseWhitespace`, making them just another normalizer. It also exposes a `getDefaultNormalizer()` function which provides the default normalization and allows for the configuration of it. Removed `matches` and `fuzzyMatches` from being publicly exposed. Updated tests, added new documentation for normalizer and getDefaultNormalizer, and removed documentation for the previous top-level `trim` and `collapseWhitespace` options.

kentcdodds

Good with just about everything :)

kentcdodds · 2018-12-10T17:09:26Z

README.md

+a string and is expected to return a normalized version of that string.
+
+Note: Specifying a value for `normalizer` _replaces_ the built-in normalization, but
+you can call `getDefaultNormalizer` to obtain a built-in normalizer, either


I'd prefer to not allow people to call this function and instead simply export it:

export const normalizer = getDefaultNormalizer({trim: true, collapseWhitespace: true})

Ok, easy to do, but then somebody wouldn't be able to get hold of, say, a default normalizer that only trims but doesn't collapse whitespace.

If you're happy with that, that's fine, and I'll hide getDefaultNormalizer and take all mention of trim and collapseWhitespace out of the docs.

then somebody wouldn't be able to get hold of, say, a default normalizer that only trims but doesn't collapse whitespace.

I'm thinking that's a pretty minimal use-case, and if it is, they just need to copy/paste this:

function getDefaultNormalizer({trim = true, collapseWhitespace = true} = {}) { return text => { let normalizedText = text normalizedText = trim ? normalizedText.trim() : normalizedText normalizedText = collapseWhitespace ? normalizedText.replace(/\s+/g, ' ') : normalizedText return normalizedText } }

Considering we're getting simplicity in exchange for a tiny bit of flexibility for 1% of use cases I think that's a win :)

I'll also have to remove the example for how to match text without trimming, for instance:
getByText(node, 'text', {normalizer: getDefaultNormalizer({trim: false})})

There'd be no easy way to do that without getDefaultNormalizer: they'd have to implement their own whitespace-collapsing normalizer. I don't have much of an opinion on that, but presumably the existing trim and collapseWhitespace options were added because somebody wanted to be able to turn them off?

Ah, that makes sense, considering I want to deprecate trim as an option. Ok, let's stick with what you have. I'd like to have @alexkrolick give this a look over too, but I'm 👍

I think some of the queries initially had different defaults for trim and collapseWhitespace, and they had different enough effects to keep separate (see #31).

I'm on the fence about exporting the factory function, but I definitely don't want to encourage copy-pasting the default implementation - that would be opting-out of future changes.

I think this is good as-is and leaves room to come up with future more streamlined APIs for other use cases.

alexkrolick · 2018-12-10T18:07:43Z

src/index.js

@@ -6,7 +6,7 @@ export * from './queries'
 export * from './wait'
 export * from './wait-for-element'
 export * from './wait-for-dom-change'
-export * from './matches'
+export {getDefaultNormalizer} from './matches'


This would be a breaking change

Yes, as I commented in the commit, I removed matches and fuzzyMatches: they looked like they were exported accidentally, and aren't documented. Are they officially supported?

They were used in jest-dom but it looks like that dependency was removed:

testing-library/jest-dom#9

testing-library/jest-dom@4c6b572

So... that means they still need to be exposed for old versions of jest-dom that are bringing in new versions of dom-testing-library?

(Actually, looking closer at those links, where were matches and fuzzyMatches used in jest-dom? I can see getNodeText, but at a quick skim I can't see matches and fuzzyMatches. It looks like they had their own implementation of matches. Am I missing something?)

Was referring to this comment testing-library/jest-dom#9 (comment)

But you're right it doesn't look like it was used directly.

@kentcdodds can you confirm whether this is a breaking change? I'm happy to change the PR so matches and fuzzyMatches aren't changed, if necessary, but it'd be preferable not to continue to expose those internals or to have to keep the same APIs for them.

I'm fine with this change.

alexkrolick · 2018-12-10T18:08:48Z

src/matches.js

-  matcher,
-  {collapseWhitespace = true, trim = true} = {},
-) {
+function matches(textToMatch, node, matcher, normalizer) {


Is there a way we can add this option without making a breaking change to the matches APIs?

I guess we can ignore this if removing the matchers as exports per #172 (comment)

alexkrolick

Temporary hold - there are breaking changes

kentcdodds · 2018-12-11T03:47:11Z

I hope that you don't mind if we merge this one first: #169 😬

kentcdodds · 2018-12-11T04:56:25Z

Let's wait for the new getByDisplayValue to get merged, then update this to account for that one as well, then we can get it merged. Thanks for your help and patience @RoystonS!

RoystonS · 2018-12-11T09:23:43Z

Sure. Will keep an eye on the getByDisplayValue PR and then give that the same normalizer treatment.

RoystonS · 2018-12-12T09:19:20Z

I'm merging in the getByDisplayValue changes now. I'll push a rebase + squash to keep this PR tidy against a newer master.

RoystonS · 2018-12-12T11:28:37Z

Rather than rebasing at this point I've pushed a merge from master and a small delta to apply normalization to queryByDisplayValue. That seems to keep existing review comments easier to follow, but I can push a squashed commit if you prefer?

It's all there now.

kentcdodds

Awesome 💯

kentcdodds · 2018-12-12T16:11:47Z

🎉 This PR is included in version 3.15.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

alexkrolick reviewed Dec 8, 2018

View reviewed changes

Expand acronyms out

ab3a203

alexkrolick previously approved these changes Dec 8, 2018

View reviewed changes

alexkrolick requested a review from kentcdodds December 9, 2018 02:58

kentcdodds reviewed Dec 9, 2018

View reviewed changes

src/matches.js Outdated Show resolved Hide resolved

RoystonS dismissed alexkrolick’s stale review via 25c531d December 10, 2018 11:30

kentcdodds reviewed Dec 10, 2018

View reviewed changes

alexkrolick reviewed Dec 10, 2018

View reviewed changes

alexkrolick requested changes Dec 10, 2018

View reviewed changes

alexkrolick previously approved these changes Dec 11, 2018

View reviewed changes

Merge branch 'master' into pr/custom-normalizer

8b41ad8

Merge remote-tracking branch 'upstream/master' into pr/custom-normalizer

c441145

Apply normalizer treatment to queryAllByDisplayValue

c51406d

RoystonS dismissed alexkrolick’s stale review via c51406d December 12, 2018 11:26

kentcdodds approved these changes Dec 12, 2018

View reviewed changes

kentcdodds merged commit a03f056 into testing-library:master Dec 12, 2018

kentcdodds added the released label Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(custom normalizer): allow custom control of normalization #172

feat(custom normalizer): allow custom control of normalization #172

RoystonS commented Dec 8, 2018

alexkrolick Dec 8, 2018

RoystonS Dec 8, 2018

kentcdodds left a comment

kentcdodds left a comment

kentcdodds Dec 10, 2018

RoystonS Dec 10, 2018

kentcdodds Dec 10, 2018

RoystonS Dec 10, 2018

kentcdodds Dec 10, 2018

alexkrolick Dec 10, 2018 •

edited

Loading

alexkrolick Dec 10, 2018

RoystonS Dec 10, 2018

alexkrolick Dec 10, 2018

RoystonS Dec 10, 2018

RoystonS Dec 10, 2018

alexkrolick Dec 10, 2018

RoystonS Dec 10, 2018

kentcdodds Dec 11, 2018

alexkrolick Dec 10, 2018

alexkrolick Dec 11, 2018

alexkrolick left a comment

kentcdodds commented Dec 11, 2018

kentcdodds commented Dec 11, 2018

RoystonS commented Dec 11, 2018 •

edited

Loading

RoystonS commented Dec 12, 2018

RoystonS commented Dec 12, 2018

kentcdodds left a comment

kentcdodds commented Dec 12, 2018

feat(custom normalizer): allow custom control of normalization #172

feat(custom normalizer): allow custom control of normalization #172

Conversation

RoystonS commented Dec 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkrolick Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkrolick left a comment

Choose a reason for hiding this comment

kentcdodds commented Dec 11, 2018

kentcdodds commented Dec 11, 2018

RoystonS commented Dec 11, 2018 • edited Loading

RoystonS commented Dec 12, 2018

RoystonS commented Dec 12, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Dec 12, 2018

alexkrolick Dec 10, 2018 •

edited

Loading

RoystonS commented Dec 11, 2018 •

edited

Loading