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

[TouchableOpacity] [0.12.X, canary] changing disabled prop crashes in test environments #1584

Closed
viggyfresh opened this issue Apr 9, 2020 · 8 comments

Comments

@viggyfresh
Copy link

The problem
Hi there! Thanks so much for all the hard work in maintaining this library. I'm trying to upgrade RNW to 0.12.2 (or beyond), and am facing an issue specific to TouchableOpacitys in Jest tests. Basically, in a test, if a Touchable's disabled property is changed, the subsequent render blows up when trying to apply the relevant styles:

TypeError: Cannot convert undefined or null to object
        at slice (<anonymous>)
        at getDOMStyleInfo (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/exports/StyleSheet/createStyleResolver.js:380:41)
        at Object.resolveWithNode (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/exports/StyleSheet/createStyleResolver.js:196:28)
        at styleResolver (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/modules/NativeMethodsMixin/index.js:80:30)
        at createDOMProps (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/modules/createDOMProps/index.js:165:24)
        at Object.setNativeProps (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/modules/NativeMethodsMixin/index.js:79:22)
        at Object.setOpacityTo (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/exports/TouchableOpacity/index.js:153:10)
        at Object._opacityInactive (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/exports/TouchableOpacity/index.js:216:10)
        at Object.TouchableOpacity_componentDidUpdate [as componentDidUpdate] (/Users/viggy/GitHub/firstopinion/javascript/node_modules/react-native-web/dist/exports/TouchableOpacity/index.js:145:12)

I dug into the stacktrace, and I think I have identified a potential culprit. In NativeMethodsMixin.js, there is a setNativeProps function which calls findNodeHandle(this). The result of that call (in tests) appears to be a ref, and thus doesn't have a classList property when node.classList is used by getDOMStyleInfo:

 <ref *1> {
      type: 'div',
      ...,
      tag: 'INSTANCE'
    }

To be clear, everything is A-ok outside of tests (in the browser, everything is fine, and node is an actual node). Happy to hop in and try to fix and file a PR - but could use some guidance as to what the actual issue is!

How to reproduce
Simplified test case: https://codesandbox.io/s/nameless-water-o6qzg?file=/src/App.test.js

Steps to reproduce:

  1. Open CodeSandbox
  2. Navigate to "run tests" tab, hit run
  3. Receive TypeError: Cannot convert undefined or null to object.

Expected behavior
No TypeError.

Environment (include versions). Did this work in previous versions?
Yep! This test works against 0.11.7 (tested in CodeSandbox), but not on 0.12.X or the canary builds 0.0.0.X.

  • React Native for Web (version): 0.12.2 and above
  • React (version): 16.13.X
  • Browser: Chrome
@necolas
Copy link
Owner

necolas commented Apr 12, 2020

Please try version 0.0.0-33f8505d6 (a canary release) and let me know if the problem is resolved. See #1538

@viggyfresh
Copy link
Author

That release works perfectly in the CodeSandbox minimal example (hooray)! Will apply it to our apps tomorrow and let you know how it fares...thanks a bunch @necolas.

@MoOx
Copy link
Contributor

MoOx commented Apr 13, 2020

@necolas btw, the canary appear as "latest" on npm https://www.npmjs.com/package/react-native-web (see version tab), you should maybe adjust the tags

@necolas
Copy link
Owner

necolas commented Apr 13, 2020

🤦‍♂️

@viggyfresh
Copy link
Author

@necolas just confirmed that this is working perfectly on the latest canary release! two quick questions:

  1. close this issue, or leave open until the fix hits master?
  2. found a similar test environment-only issue related to ScrollView refs - do you prefer me filing a separate issue or just a comment + CodeSandbox in The future of React Native for Web #1538?

@viggyfresh
Copy link
Author

and, most importantly - thank you

@necolas
Copy link
Owner

necolas commented Apr 15, 2020

Yeah let's leave it open until it's fixed on master. And yes please a new comment about the ScrollView issue would be great

@viggyfresh
Copy link
Author

Yeah let's leave it open until it's fixed on master. And yes please a new comment about the ScrollView issue would be great

done and done! thanks again.

@necolas necolas added this to the 0.13 milestone Apr 16, 2020
@necolas necolas closed this as completed Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants