-
Notifications
You must be signed in to change notification settings - Fork 274
feat: add text match options a.k.a string precision API #541
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 text match options a.k.a string precision API #541
Conversation
cc @TAGraves |
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.
This is great, thank you! Left a few notes to address, but this is pretty close
@@ -53,6 +53,9 @@ type ReactTestInstance = { | |||
}; | |||
``` | |||
|
|||
### Options | |||
Query first argument can be a **string** or a **regex**. Some queries accept optional argument which change string matching behaviour. See [TextMatch](api-queries#textmatch) for more info. |
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.
Can you run Prettier over this file?
src/helpers/getByAPI.js
Outdated
const filterNodeByType = (node, type) => node.type === type; | ||
|
||
const getNodeByText = (node, text) => { | ||
const matchText = ( |
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.
Let's use the original implementation, which can be found here: https://github.com/testing-library/native-testing-library/blob/master/src/lib/matches.js. This will also solve the missing normalization, which I believe we can implement, even just to be more compatible with testing library in general, it's not too much code really
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, I'll go with that implementation but in slightly modified manner: we don't have a matcher defined as predicate function option implemented
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.
Overall looks good, I've added some style & RTL-compat comments
waitForOptions?: WaitForOptions, | ||
options?: TextMatchOptions, |
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.
It seems that RTL uses different args order for all findBy queries: text, queryOptions, waitForOptions. I think we should use the same order.
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.
This was intentional decision, because switching arguments would be a breaking change. Maybe worth to have this feature in the next major? I doubt many use waitForOptions at all so maybe we could get away with this, but maybe not
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 generally think that 1) findBy is least used query type of [getBy, queryBy, findBy] and 2) queryOptions are more used than waitForOptions, as some users seem to prefer { exact: false}
over /regex/
query.
Overall, I am for breaking change with minor version bump, and docs note.
As a help for the users, we could emit error/warning if matchOptions contain fields from waitForOptions (timeout, interval) to ease the transition.
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.
It's kinda missing feature we should have shipped in v7 anyway...
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 I've got it, as a compat feature till next major release we would inspect queryOptions
for timeout
and interval
props and use them if they exist, but issue a console warning asking user to migrate their code.
This won't break the older code, but will notify the user about API change. For us it means a relatively small change in next major release (vs alternative of re-ordering the queryOptions
and waitForOptions
args for all potential users).
WDYT?
typings/index.d.ts
Outdated
@@ -64,19 +65,22 @@ interface GetByAPI { | |||
} | |||
|
|||
interface QueryByAPI { | |||
queryByText: (name: string | RegExp) => ReactTestInstance | null; | |||
queryByText: (name: string | RegExp, options?: TextMatchOptions) => ReactTestInstance | null; |
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.
RTL in their docs (but curiously not their ts typesing code) have a type TextMatch = string | RegExp, maybe we could use the same.
src/helpers/findByAPI.js
Outdated
|
||
export const findAllByPlaceholderText = (instance: ReactTestInstance) => ( | ||
placeholder: string | RegExp, | ||
waitForOptions: WaitForOptions = {} | ||
waitForOptions: WaitForOptions = {}, | ||
textMatchOptions?: TextMatchOptions |
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.
textMatchOptions?: TextMatchOptions | |
queryOptions?: TextMatchOptions |
Consider naming textMatchOptions as queryOptions it match RTL docs (please change in other places as well).
typings/index.d.ts
Outdated
@@ -325,6 +335,10 @@ export declare const render: ( | |||
export declare const cleanup: () => void; | |||
export declare const fireEvent: FireEventAPI; | |||
|
|||
type TextMatchOptions = { | |||
exact: boolean, |
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.
exact: boolean, | |
exact?: boolean, |
src/matches.js
Outdated
typeof trim !== 'undefined' || | ||
typeof collapseWhitespace !== 'undefined' |
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.
It's cheaper to not typeof
typeof trim !== 'undefined' || | |
typeof collapseWhitespace !== 'undefined' | |
trim !== undefined || collapseWhitespace !== undefined |
If you want to prevent that normalization, or provide alternative normalization (e.g. to remove Unicode control characters), you can provide a `normalizer` function in the options object. This function will be given a string and is expected to return a normalized version of that string. | ||
|
||
:::info | ||
Specifying a value for `normalizer` replaces the built-in normalization, but you can call `getDefaultNormalizer` to obtain a built-in normalizer, either to adjust that normalization or to call it from your own normalizer. |
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.
you can't call getDefaultNormalizer
because it's not exported from the library entrypoint
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 was working on a parallel PR #546, to enable partial matching (somehow I thought that {exact: false}
was the default behaviour. As suggested by @thymikee I closed my PR but identified two tests that I would expect to behave differently. If you need any help, feel free to ask or look at my implementation, were I got those to pass.
Edit: My comments are tagged as request changes, it's of course a suggestion, if needed I can help to get them in a second PR.
src/__tests__/queryByApi.test.js
Outdated
|
||
const { queryByText } = render(<NestedTexts />); | ||
|
||
expect(queryByText('My text', { exact: false })?.props.nativeID).toBe('1'); |
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.
Shouldn't it be the opposite? Let's say that I have a onPress handler on the Text of id '2'
, if I fireEvent.press(getByText('My text', { exact: false }))
, I'd expect the handler to be called. Am I misunderstanding something?
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.
On web this test returns the element with nativeId=2
}) | ||
).toBeTruthy(); | ||
}); | ||
}); |
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 would expect this test to pass. This is would be consistent with the web behaviour. It currently fails in this PR.
test('queryAllByText does not match several times the same text', () => {
const allMatched = render(
<Text nativeID="1">
Start
<Text nativeID="2">This is a long text</Text>
</Text>
).queryAllByText('long text', { exact: false });
expect(allMatched.length).toBe(1);
expect(allMatched[0].props.nativeID).toBe('2');
});
Co-authored-by: Maciej Jastrzebski <[email protected]>
@AugustinLF could you try to run your tests against most current code from this PR? I think I resolved the problem you've mentioned in your review. |
try { | ||
return instance.find((node) => getNodeByText(node, text, options)); | ||
const matches = instance.findAll((node) => |
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.
Hm, seems like we could include this logic in getAllByText
and reuse here? It's what we do with ByTestId
queries. Otherwise we get different behavior between getByText
and getAllByText
.
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.
Yup good point, definitely gonna implement this right away
@RafikiTiki The first case (matching the deepest one) now passes, I still have to failing ones regarding queryAll test('queryAllByText does not match several times the same text', () => {
const allMatched = render(
<Text nativeID="1">
Start
<Text nativeID="2">This is a long text</Text>
</Text>
).queryAllByText('long text', { exact: false });
expect(allMatched.length).toBe(1); // Errors, gets 2
expect(allMatched[0].props.nativeID).toBe('2');
});
test('queryAllByText matches all the matching nodes', () => {
const allMatched = render(
<Text nativeID="1">
Start
<Text nativeID="2">This is a long text</Text>
<Text nativeID="3">This is another long text</Text>
</Text>
).queryAllByText('long text', { exact: false });
expect(allMatched.length).toBe(2); // Errors, gets 3
expect(allMatched.map((node) => node.props.nativeID)).toEqual(['2', '3']);
}); |
@RafikiTiki If you need a hand on this PR, that would be a pleasure to help to get this one in 😄 |
@AugustinLF Yup, I'd love to get some help on this PR as I started working in a full time project again this week so I won't have time to quickly finish this up. I've already talked with @thymikee last week about issues we're facing with current implementation and the main problem is the way we are getting text content of nested |
@RafikiTiki I will do so. I will cherry-pick your commits, look about removing the recursive look-up, and see how the tests fare! |
BUMP for this PR, would really help a lot of us stuck in v6.0 to migrate to the latest version. |
@janithl, yeah I totally dropped the ball on this one, other priorities. I still have that in the back of my mind, but I see that the code changed quite a lot since, so I'll see if I can squeeze that. But if you want to pick it up, don't hesitate! |
Summary
As mentioned in #514 current implementation lacked options for text matching customisation. This PR bridges this gap and provides smoother migration experience for users coming from native-testing-library.
Things that need further discussion
This is a basic implementation with just one option allowing to opt-in for a partial and/or case-insensitive string match. NTL had another option for text normalisation customisation which this PR currently lacks. It's up for debate if we need this feature.
Another thing to consider is API consistency: to avoid pushing breaking change to existing RNTL users I've put TextSearchOptions param as the last one (even in findBy* queries, coming after waitFor options; in NTL findBy* queries this param was coming second to last, before waitFor options).