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

Fix ScrollView getInnerViewNode and getInnerViewRef ref methods #30588

Closed
wants to merge 3 commits into from

Conversation

vshab
Copy link
Contributor

@vshab vshab commented Dec 14, 2020

Summary

Currently ScrollView ref's getInnerViewNode and getInnerViewRef are unbound and don't work as expected returning empty values. The origin of this problem probably is issued by d2f314a

Working example of the problem is available here: https://github.com/vshab/RNGetInnerViewBug

This PR binds getInnerViewNode and getInnerViewRef to ScrollView and adds test checking the value of getInnerViewRef.

Before:
Simulator Screen Shot - iPhone 11 - 2020-12-15 at 02 07 03
After:
Simulator Screen Shot - iPhone 11 - 2020-12-15 at 01 49 31

Changelog

[JavaScript] [Fixed] - ScrollView: Fix getInnerViewNode and getInnerViewRef ref methods

Test Plan

  1. The test checking the value of getInnerViewRef is added.
  2. Example app demonstrating the problem is provided with before/after screenshots.

@facebook-github-bot
Copy link
Contributor

Hi @vshab!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@react-native-bot react-native-bot added the No CLA Authors need to sign the CLA before a PR can be reviewed. label Dec 15, 2020
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 2a175b1

@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 Dec 15, 2020
@react-native-bot react-native-bot removed the No CLA Authors need to sign the CLA before a PR can be reviewed. label Dec 15, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@elicwhite
Copy link
Member

The other methods aren't bound either are they? Is the behavior of this method inconsistent with the behavior of the other methods? The functions on refs in React Native are pretty much all not bound to avoid creating new functions or breaking referential equality on every render

@vshab
Copy link
Contributor Author

vshab commented Dec 18, 2020

@TheSavior
Hello!

The other methods aren't bound either are they?

Other methods are bound because they are defined as arrow function class properties. See https://github.com/facebook/react-native/blob/master/Libraries/Components/ScrollView/ScrollView.js#L802 for example

Is the behavior of this method inconsistent with the behavior of the other methods?

Yes. Currently getInnerViewNode and getInnerViewRef are just unusable. Check the example: https://github.com/vshab/RNGetInnerViewBug
I think it's a regression introduced by d2f314a. It used to work before

The functions on refs in React Native are pretty much all not bound to avoid creating new functions or breaking referential equality on every render

As I said before all other methods are arrow function preserving this and only getInnerViewNode and getInnerViewRef are regular class methods so they have different call behaviour I think.

@elicwhite
Copy link
Member

Should the fix be to make these functions arrow functions as well instead of .bind()?

@vshab
Copy link
Contributor Author

vshab commented Dec 19, 2020

If you say so :)
I just wanted to make a change that affects as little parts as possible.

@vshab
Copy link
Contributor Author

vshab commented Dec 19, 2020

@TheSavior I replaced bind with arrow functions.

@vshab
Copy link
Contributor Author

vshab commented Jan 14, 2021

@TheSavior Hello!

Anything else should be done to merge it?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@kacieb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kacieb merged this pull request in 6e36d04.

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

Successfully merging this pull request may close these issues.

5 participants