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 ScrollView StrictMode compatible #24214

Closed
wants to merge 1 commit into from

Conversation

Jyrno42
Copy link
Contributor

@Jyrno42 Jyrno42 commented Mar 30, 2019

Summary

Converts ScrollView to use constructor instead of UNSAFE_componentWillMount and componentDidUpdate instead of UNSAFE_componentWillReceiveProps.

see #22186

Changelog

[General] [Fixed] - Converted ScrollView to be compatible with StrictMode

Test Plan

Tested manually via RNTester by opening StrictMode example and ensuring no strict-mode warnings are rendered. I also verified in a fresh project that stickyHeaderIndices still works.

Converts ScrollView to use constructor instead of UNSAFE_componentWillMount and componentDidUpdate
instead of UNSAFE_componentWillReceiveProps.

Related to facebook#22186
@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
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Mar 30, 2019

Note: The ScrollView uses ReactNative.findNodeHandle in some places. I could not figure out how to remove those calls.

@cpojer
Copy link
Contributor

cpojer commented Apr 3, 2019

Similarly to the other PR, just moving lifecycle functionality to the constructor won't actually fix real issues. We'll need to change this component, possibly to use hooks. Would you be interested in reworking ScrollView with that in mind? Keep in mind that we have to be very careful and do a good amount of testing so we don't break this component.

@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Apr 16, 2019

Got it. I'll try to get animated component one done/merged first, and if anyone else has not grabbed scrollview conversion by then I'll take it as well.

@Jyrno42 Jyrno42 closed this Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: ScrollView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants