Skip to content

Make AnimatedComponents and Touchables strict mode compatible - #24218

Closed
Jyrno42 wants to merge 1 commit into
react:masterfrom
Jyrno42:strict-mode-touchables-only
Closed

Make AnimatedComponents and Touchables strict mode compatible#24218
Jyrno42 wants to merge 1 commit into
react:masterfrom
Jyrno42:strict-mode-touchables-only

Conversation

@Jyrno42

Copy link
Copy Markdown
Contributor

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-botfacebook-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

@analysis-botanalysis-bot left a comment

Copy link
Copy Markdown

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.

Comment threadLibraries/Components/Touchable/TouchableBounce.js Outdated
Comment threadLibraries/Components/Touchable/TouchableOpacity.js Outdated

@analysis-botanalysis-bot left a comment

Copy link
Copy Markdown

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.

Comment threadLibraries/Components/Touchable/TouchableBounce.js Outdated
Comment threadLibraries/Components/Touchable/TouchableOpacity.js Outdated
@Jyrno42
Jyrno42force-pushed the strict-mode-touchables-only branch from 386dc80 to 0215319CompareMarch 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 react#22186
@Jyrno42
Jyrno42force-pushed the strict-mode-touchables-only branch from 0215319 to 4b0c955CompareMarch 30, 2019 21:17
@Jyrno42Jyrno42 mentioned this pull request Mar 30, 2019
@Jyrno42

Copy link
Copy Markdown
ContributorAuthor

Will look into the test failures tomorrow.

@cpojer

Copy link
Copy Markdown
Contributor

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!

@cpojercpojer closed this Apr 3, 2019
@Jyrno42

Copy link
Copy Markdown
ContributorAuthor

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

@Jyrno42

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@gaearon

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: AnimatedCLA SignedThis 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

@Jyrno42@cpojer@gaearon@analysis-bot@facebook-github-bot@react-native-bot