-
Notifications
You must be signed in to change notification settings - Fork 472
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(custom normalizer): allow custom control of normalization #172
Changes from all commits
abd75dc
ab3a203
25c531d
8b41ad8
c441145
c51406d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,28 @@ | ||
import {fuzzyMatches, matches} from '../' | ||
import {fuzzyMatches, matches} from '../matches' | ||
|
||
// unit tests for text match utils | ||
|
||
const node = null | ||
const normalizer = str => str | ||
|
||
test('matchers accept strings', () => { | ||
expect(matches('ABC', node, 'ABC')).toBe(true) | ||
expect(fuzzyMatches('ABC', node, 'ABC')).toBe(true) | ||
expect(matches('ABC', node, 'ABC', normalizer)).toBe(true) | ||
expect(fuzzyMatches('ABC', node, 'ABC', normalizer)).toBe(true) | ||
}) | ||
|
||
test('matchers accept regex', () => { | ||
expect(matches('ABC', node, /ABC/)).toBe(true) | ||
expect(fuzzyMatches('ABC', node, /ABC/)).toBe(true) | ||
expect(matches('ABC', node, /ABC/, normalizer)).toBe(true) | ||
expect(fuzzyMatches('ABC', node, /ABC/, normalizer)).toBe(true) | ||
}) | ||
|
||
test('matchers accept functions', () => { | ||
expect(matches('ABC', node, text => text === 'ABC')).toBe(true) | ||
expect(fuzzyMatches('ABC', node, text => text === 'ABC')).toBe(true) | ||
expect(matches('ABC', node, text => text === 'ABC', normalizer)).toBe(true) | ||
expect(fuzzyMatches('ABC', node, text => text === 'ABC', normalizer)).toBe( | ||
true, | ||
) | ||
}) | ||
|
||
test('matchers return false if text to match is not a string', () => { | ||
expect(matches(null, node, 'ABC')).toBe(false) | ||
expect(fuzzyMatches(null, node, 'ABC')).toBe(false) | ||
expect(matches(null, node, 'ABC', normalizer)).toBe(false) | ||
expect(fuzzyMatches(null, node, 'ABC', normalizer)).toBe(false) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be a breaking change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, as I commented in the commit, I removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They were used in jest-dom but it looks like that dependency was removed: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So... that means they still need to be exposed for old versions of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Actually, looking closer at those links, where were There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was referring to this comment testing-library/jest-dom#9 (comment) But you're right it doesn't look like it was used directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kentcdodds can you confirm whether this is a breaking change? I'm happy to change the PR so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with this change. |
||
export * from './get-node-text' | ||
export * from './events' | ||
export * from './get-queries-for-element' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to not allow people to call this function and instead simply export it:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 oftrim
andcollapseWhitespace
out of the docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that's a pretty minimal use-case, and if it is, they just need to copy/paste this:
Considering we're getting simplicity in exchange for a tiny bit of flexibility for 1% of use cases I think that's a win :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 existingtrim
andcollapseWhitespace
options were added because somebody wanted to be able to turn them off?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.