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

Make AnimatedComponents and Touchables strict mode compatible #24218

Closed
wants to merge 1 commit into from

Conversation

Jyrno42
Copy link
Contributor

@Jyrno42 Jyrno42 commented Mar 30, 2019

Summary

Convert all Touchables to be class based and remove UNSAFE props. Also renamed
Touchable.Mixin.withoutDefaultFocusAndBlur to Touchable.MixinWithoutDefaultFocusAndBlur
to improve flow automatic typings.

Note: TouchableNativeFeedback uses ReactNative.findNodeHandle which triggers a
warning in strict mode during a tap. I could not figure out how to remove the need for
that call.

Related to #22186

Changelog

[General] [Fixed] - Converted createAnimatedComponent to be compatible with StrictMode
[General] [Fixed] - Converted Touchable.Mixin to be compatible with StrictMode and updated built-in Touchable components.
[General] [Changed] - Renamed Touchable.Mixin.withoutDefaultFocusAndBlur to Touchable.MixinWithoutDefaultFocusAndBlur

Test Plan

Tested manually via RNTester by opening StrictMode example and ensuring no strict-mode warnings are rendered. Also made sure onPress handlers and effects still work for all of them.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 30, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/Touchable/TouchableBounce.js Outdated Show resolved Hide resolved
Libraries/Components/Touchable/TouchableOpacity.js Outdated Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/Touchable/TouchableBounce.js Outdated Show resolved Hide resolved
Libraries/Components/Touchable/TouchableOpacity.js Outdated Show resolved Hide resolved
@Jyrno42 Jyrno42 force-pushed the strict-mode-touchables-only branch from 386dc80 to 0215319 Compare March 30, 2019 20:50
Convert all Touchables to be class based and remove UNSAFE props. Also renamed
Touchable.Mixin.withoutDefaultFocusAndBlur to Touchable.MixinWithoutDefaultFocusAndBlur
to improve flow automatic typings.

Note: TouchableNativeFeedback uses ReactNative.findNodeHandle which triggers a
warning in strict mode during a tap. I could not figure out how to remove the need for
that call.

Related to facebook#22186
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Mar 30, 2019

Will look into the test failures tomorrow.

@cpojer
Copy link
Contributor

cpojer commented Apr 3, 2019

Thank you for this PR! Moving function calls from unsafe lifecycle methods to the constructor unfortunately doesn't actually fix any issues, it just masks them further.

For Touchable, we are hoping to move those out of React Native core as part of the Lean Core effort, but first we will need to introduce a replacement. For now, we are not looking to make any larger changes to it that may break them.

Would you mind sending a new PR that just fixes the createAnimatedComponent and in the proper way? Thank you!

@cpojer cpojer closed this Apr 3, 2019
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Apr 3, 2019

So I should convert the animated component to use hooks too like you recommended in the scrollview PR?

@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Apr 3, 2019

Also this might be more of a support question so I don't expect an answer, but why isn't calling attachProps from constructor the same as doing it in UNSAFE_componentWillMount? Given that it essentially only sets up the _propsAnimated value in the first pass.

Note: I do see the problem with the willReceiveProps and didUpdate case (attachProps call used to run before render, now it runs after it).

@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Apr 3, 2019

@cpojer Can you clarify the above things for me if possible

@gaearon
Copy link
Collaborator

gaearon commented Apr 3, 2019

why isn't calling attachProps from constructor the same as doing it in UNSAFE_componentWillMount?

It is the same.

But I haven't looked in the code in detail so I don't know for sure whether it's a problem or not.

Just moving all code from UNSAFE_componentWillMount to constructor doesn't actually make it safe. To make it "safe" we need to make sure that there's no side effects. If attachProps doesn't have side effects then it's okay to call it in the constructor.

@gaearon
Copy link
Collaborator

gaearon commented Apr 3, 2019

In particular, a PR that moves things from unsafe_componentWillMount into constructor needs to include an analysis of why it is safe (e.g. you verified that there are no observable side effects or mutations or unreleased resources in any functions currently being called from unsafe_componentWillMount).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants