fix(breaking): correct faulty implementation of finding by name #62
Conversation
| @@ -76,7 +76,8 @@ test('getByName, queryByName', () => { | |||
| sameButton.props.onPress(); | |||
|
|
|||
| expect(bananaFresh.props.children).toBe('not fresh'); | |||
| expect(() => getByName('InExistent')).toThrow(); | |||
| expect(() => getByName('InExistent')).toThrow('No instances found'); | |||
| expect(() => getByName(Text)).toThrow('Expected 1 but found 3'); | |||
There was a problem hiding this comment.
fixed errors btw, so it doesn't incorrectly say "No instances found" when there's more than 1
src/helpers/getByAPI.js
Outdated
| ); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Currently the only supported library to search by text is \`react-native\`.\n\n${error}` |
There was a problem hiding this comment.
Can you extract constructing this string to a method? Or even create custom Error?
src/helpers/getByAPI.js
Outdated
| import ErrorWithStack from './errorWithStack'; | ||
|
|
||
| const getNodeByType = (node, type) => node.type === type; |
There was a problem hiding this comment.
I think it will be better to name getNodeByName and getNodeByType as filterNodeByName and filterNodeByType. The getNode method indicates that it returns Node, but in fact it returns Boolean
| const getNodeByText = (node, text) => { | ||
| try { | ||
| // eslint-disable-next-line | ||
| const { Text, TextInput } = require('react-native'); |
There was a problem hiding this comment.
As I understand, we make this require call here to make lib compatible with other environments?
There was a problem hiding this comment.
There may be different text primitives in different environments (like ReactVR). I stand that it's safer to assert on right primitives instead of using string-based comparisons, where the display name may change over time.
| const { Text, TextInput } = require('react-native'); | ||
| return ( | ||
| (getNodeByType(node, Text) || getNodeByType(node, TextInput)) && | ||
| (typeof text === 'string' |
There was a problem hiding this comment.
typeof something === 'string' is used couple of times throughout the code - can you extract it to method?
There was a problem hiding this comment.
Wish I could, but Flow looses type information because of fn extraction :<
| @@ -13,11 +13,11 @@ exports[`shallow rendering React Test Instance 1`] = ` | |||
| `; | |||
|
|
|||
| exports[`shallow rendering React elements 1`] = ` | |||
| <Component> | |||
| <Component | |||
| <View> | |||
There was a problem hiding this comment.
This looks much better C:
|
Addressed, thanks! |
Summary
Migrating from RN 0.57.4 -> 0.57.5 revealed a flaw in the implementation of our helper to get nodes by name. I believe now we should have never test against
node.typedirectly if it's a string, as this compares to the host component type and may result in finding more nodes than actually expected (which in RN versions prior to 0.57.5 allowed us to dogetByName('Text')orgetByName('View')where they didn't havetype.displayNamenortype.nameset correctly).This is breaking but since it fixes the behavior to correct one I'm keen on releasing this as a patch.
Migration path:
rename:
getByName('Text')->getByName(Text)getByName('View')->getByName(View)getByName('Image')->getByName(Image)getByName('Modal')->getByName(Modal)While working on this I realized that getting by string name is very fragile in React ecosystem. Tests written this way may break as soon as you (or authors of libraries you use) wrap components with a HOC or
forwardRef. This is a problem I've seen in Enzyme as well (usingshallow.findAPI) but I belittled it.Since now on we'll be discouraging usage of
getByName(string)as dangerous/fragile API and introduce a new helper calledgetByTypewhich will do the same asgetByName(ReactComponent).BTW fixed incorrect error message when more than 1 element was found by
getBy*methods.Test plan
Tests green on RN 0.57.5