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 connection of animated nodes and scroll offset with useNativeDriver. #24177

Conversation

msand
Copy link
Contributor

@msand msand commented Mar 28, 2019

Add example showing regression before this fix is applied.

Summary

#18187 Was found to introduce a regression in some internal facebook code-base end to end test which couldn't be shared. I was able to create a reproducible demo of a regression I found, and made a fix for it. Hopefully this will fix the internal test, such that the pr can stay merged.

Changelog

[GENERAL] [Fixed] - Fix connection of animated nodes and scroll offset with useNativeDriver.

Test Plan

The commit includes a new example for RNTester. Unit and E2E integration could be added in a later commit.

ping @cpojer

@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 28, 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:

  • flow found some issues.

RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
… when using native driver in Animated, fix Expected node to be marked as "native", optimize AnimatedNode creation and connections""

This reverts commit 95c7db9
@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from f25b657 to 80e5a9e Compare March 28, 2019 03:21
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:

  • flow found some issues.

RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from 80e5a9e to f7507e2 Compare March 28, 2019 03:23
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:

  • flow found some issues.

RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from f7507e2 to 827fc61 Compare March 28, 2019 03:27
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:

  • flow found some issues.

RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
RNTester/js/ScrollViewAnimatedExample.js Outdated Show resolved Hide resolved
@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from 827fc61 to f2643d1 Compare March 28, 2019 03:29
Add example showing regression before this fix is applied.
@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from f2643d1 to ab49cb0 Compare March 28, 2019 03:37
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.

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

@cpojer
Copy link
Contributor

cpojer commented Apr 9, 2019

Let me get back to this one!

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.

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

@msand msand force-pushed the fix-native-animated-scroll-offset-connection branch from ace654a to 19bba43 Compare April 16, 2019 01:43
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.

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

@matpaul
Copy link
Contributor

matpaul commented Jun 4, 2019

@cpojer Hello! what stop to merge this pr?)
Tested it with patch in production everything work very good (morphing 'svg' with nativeDriver awesome!)

@cpojer
Copy link
Contributor

cpojer commented Jun 5, 2019

I finally got a strong enough computer to run the tests locally at FB :D I rebased this internally at FB and can verify that as far as I can see the previous issue should be resolved. I'm trying to land it now.

@msand
Copy link
Contributor Author

msand commented Jun 5, 2019

@cpojer I made a small change to optimize the connection queue handling

Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

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

Thanks for adding an RNTester page that captures this use case!

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @msand in bdc530b.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 6, 2019
facebook-github-bot pushed a commit that referenced this pull request Jun 13, 2019
Summary:
Change O(n^2) to O(n)

Minor follow-up to: #24177

## Changelog

[Internal] [Changed] - Optimize native animated connection queue handling
Pull Request resolved: #25256

Differential Revision: D15804527

Pulled By: cpojer

fbshipit-source-id: 4a1e1b51faf6ed7b98eb08aa47e18cfaea541dad
EvanBacon added a commit to EvanBacon/react-native-web that referenced this pull request Sep 17, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
…er. (facebook#24177)

Summary:
Add example showing regression before this fix is applied.

facebook#18187 Was found to introduce a regression in some internal facebook code-base end to end test which couldn't be shared. I was able to create a reproducible demo of a regression I found, and made a fix for it. Hopefully this will fix the internal test, such that the pr can stay merged.

## Changelog

[GENERAL] [Fixed] - Fix connection of animated nodes and scroll offset with useNativeDriver.
Pull Request resolved: facebook#24177

Reviewed By: rickhanlonii

Differential Revision: D14845617

Pulled By: cpojer

fbshipit-source-id: 1f121dbe773b0cde2adf1ee5a8c3c0266034e50d
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Change O(n^2) to O(n)

Minor follow-up to: facebook#24177

## Changelog

[Internal] [Changed] - Optimize native animated connection queue handling
Pull Request resolved: facebook#25256

Differential Revision: D15804527

Pulled By: cpojer

fbshipit-source-id: 4a1e1b51faf6ed7b98eb08aa47e18cfaea541dad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated 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.

8 participants