fix: Patch AnimatedStyle to avoid discarding the initial style info by gabrieldonadel · Pull Request #35198 · react/react-native

gabrieldonadel · 2022-11-04T02:12:08Z

Summary

This PR patches AnimatedStyle to avoid discarding the initial style information which destroys the output of web-style compilers and breaks rendering, as requested on #34425. This uses a slightly modified version of a patch used by react-native-web necolas/react-native-web@4c678d2.

Changelog

[General] [Fixed] - Patch AnimatedStyle to avoid discarding the initial style info

Test Plan

Run yarn jest Animated and ensure CI is green

analysis-bot · 2022-11-04T02:34:42Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,070,315	-31,948
android	hermes	armeabi-v7a	6,442,768	-28,676
android	hermes	x86	7,485,557	-34,042
android	hermes	x86_64	7,345,103	-32,923
android	jsc	arm64-v8a	8,934,976	-32,057
android	jsc	armeabi-v7a	7,669,092	-28,785
android	jsc	x86	8,995,029	-34,166
android	jsc	x86_64	9,473,961	-33,049

Base commit: e504141
Branch: main

analysis-bot · 2022-11-04T03:04:05Z

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

Base commit: e504141
Branch: main

pull-bot · 2022-11-04T03:17:04Z

PR build artifact for bdc981993f6df4424ac597c03b0b4db57d2ee460 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

javache · 2022-11-04T09:29:31Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

gabrieldonadel · 2022-11-04T12:30:20Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

@necolas would you mind helping me with this one? I know that the problem is described on necolas/react-native-web#2387 but I'm not completely sure how to recreate that scenario on this repo

necolas · 2022-11-04T17:36:01Z

On web, using flattenStyle creates a new object each time which causes correctness and perf issues:
It mixes inline and compiled styles into a single object.
The style runtime can no longer recognize the style object as static / pre-compiled.
The style runtime can no longer cache the result of style merges involving this style.

necolas · 2022-11-04T17:38:23Z

Since Animated nodes can only exist on inline styles, this function plucks them out of the provided styles and places them into a separate object that can be appended to the original input styles.

necolas · 2022-11-04T17:47:16Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

The behavior is broken on web because using flattenStyle is fundamentally incompatible with any approach to compiling styles to CSS. necolas/react-native-web#2387

A unit test for this change in RN would only show that Animated nodes are duplicated in a new style object appended to the input styles, and that the return from the Style node is an array instead of an object. There would be no functional difference on native.

pull-bot · 2022-11-08T04:57:15Z

PR build artifact for c47a9f4750ead68dd966168feb1682c81c5286b3 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-08T12:55:11Z

PR build artifact for c45bf58f477128dd5db35347065bb731c8874a89 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-09T00:01:54Z

PR build artifact for b45837a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

javache · 2022-11-10T14:48:51Z

Does this correctly handle scenarios like this?

style={[
{backgroundColor: animatedValue},
{backgroundColor: 'red'},
]}

The expected outcome would be that animatedValue is not used.

necolas · 2022-11-10T17:39:52Z

Yes, it does. The first thing we do is a flatten, and then extract any animated values that are left

pull-bot · 2022-11-11T04:49:12Z

PR build artifact for 45e8c02 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-11-12T21:26:49Z

@necolas FYI this should be good for another review round

necolas · 2022-11-14T18:26:24Z

+ it('does not discard initial style', () => {
+ const value1 = new Animated.Value(1);
+ const scale = value1.interpolate({
+ inputRange: [0, 1],
+ outputRange: [1, 2],
+ });
+ const callback = jest.fn();
+ const node = new AnimatedProps(
+ {
+ style: {
+ transform: [
+ {
+ scale,
+ },
+ ],
+ },
+ },
+ callback,
+ );


This test will be a bit confusing without changes / explanation. The behavior matters when the input style is a mix of values from StyleSheet.create (ie contains no animated values, could be optimized to another format) and an inline style with an animation

Sounds good, I've updated tests a bit to include StyleSheet.create

pull-bot · 2022-11-16T05:07:00Z

PR build artifact for 5b6fede is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-17T06:04:07Z

PR build artifact for 650c45e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-17T16:14:40Z

PR build artifact for 11daeda is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-11-17T17:45:34Z

Hi @cipolleschi, do you mind importing this PR? 😅

facebook-github-bot · 2022-11-17T17:58:20Z

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

…41176) Summary: #35198 introduced a regression where if an `{transform: undefined}` style is provided to an Animated View a `Cannot read property 'map' of undefined` type error is thrown <img src="https://github.com/facebook/react-native/assets/11707729/bb87781e-1ba7-40ec-879d-a57cef3e10d9" height="200" /> ## Changelog: [GENERAL] [FIXED] - Fix `createAnimatedStyle` when providing an undefined transform style Pull Request resolved: #41176 Test Plan: <details> <summary>Render an `Animated.View` passing `style={{transform: undefined}}`</summary> E.g. ``` const UndefinedTransform = () => { return ( <View> <Animated.View style={{transform: undefined}} /> </View> ); }; ``` </details> ### RNTester 1. Open the RNTester app and navigate to the Animated page 2. Navigate to the Transform Styles page 3. App should not throw any errors <table> <tr><th>Before</th><th>After</th></tr> <tr> <td><video src="https://github.com/facebook/react-native/assets/11707729/92ba9c3b-60b0-4805-8080-0e7fb7c00345"/></td> <td><video src="https://github.com/facebook/react-native/assets/11707729/80e2bba8-6ff6-4cf5-bcb8-26de0b869036"/></td> </tr> </table> Reviewed By: fabriziocucci Differential Revision: D50638415 Pulled By: javache fbshipit-source-id: 0ee949f019a77b8bef557888694e0e8404810105

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 Nov 4, 2022

react-native-bot added the Bug label Nov 4, 2022

necolas reviewed Nov 4, 2022
View reviewed changes

necolas mentioned this pull request Nov 4, 2022
[w3c] ☂️ Web Styles (Part 1) umbrella issue #34425
Closed
74 tasks

gabrieldonadel force-pushed the fix/patch-animated-style branch from bdc9819 to c47a9f4Compare November 8, 2022 03:53

gabrieldonadel requested a review from necolas November 8, 2022 03:54

gabrieldonadel force-pushed the fix/patch-animated-style branch from ba6f59d to c45bf58Compare November 8, 2022 12:24

necolas mentioned this pull request Nov 8, 2022
☂️ npm packages for react-native-web #35263
Closed
13 tasks

gabrieldonadel added 4 commits November 8, 2022 20:30

fix: Patch AnimatedStyle to avoid discarding the initial style info
8172721

chore: Update AnimatedStyle __getValue to return an array of objects
8d4717e

chore: Fix unit tests
986a0c2

chore: Add Platform.OS check to AnimatedStyle
b45837a

gabrieldonadel force-pushed the fix/patch-animated-style branch from 92af6c4 to b45837aCompare November 8, 2022 23:30

necolas reviewed Nov 9, 2022
View reviewed changes

Comment threadLibraries/Animated/__tests__/Animated-web-test.js Outdated
Comment threadLibraries/Animated/nodes/AnimatedStyle.js Outdated
Comment threadLibraries/Animated/nodes/AnimatedStyle.js Outdated

chore: Update AnimatedStyle and Animated tests
45e8c02

gabrieldonadel force-pushed the fix/patch-animated-style branch from 6999082 to 45e8c02Compare November 11, 2022 03:43

gabrieldonadel requested a review from necolas November 11, 2022 03:43

necolas approved these changes Nov 14, 2022
View reviewed changes

chore: Remove unnecessary mock from test
5b6fede

chore: Update Animated web tests
650c45e

gabrieldonadel requested a review from javache November 17, 2022 04:56

javache approved these changes Nov 17, 2022
View reviewed changes

Comment threadLibraries/Animated/nodes/AnimatedStyle.js Outdated

chore: Update AnimatedStyle constructor
11daeda

facebook-github-bot closed this in c06323fNov 18, 2022

gabrieldonadel deleted the fix/patch-animated-style branch November 18, 2022 15:44

gabrieldonadel mentioned this pull request Oct 24, 2023
Fix createAnimatedStyle when providing undefined transform style #41176
Closed

Uh oh!

Conversation

gabrieldonadel commented Nov 4, 2022

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Nov 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Nov 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Nov 4, 2022

Uh oh!

javache commented Nov 4, 2022

Uh oh!

gabrieldonadel commented Nov 4, 2022

Uh oh!

necolasNov 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

necolasNov 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

necolas commented Nov 4, 2022

Uh oh!

pull-bot commented Nov 8, 2022

Uh oh!

pull-bot commented Nov 8, 2022

Uh oh!

pull-bot commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javache commented Nov 10, 2022

Uh oh!

necolas commented Nov 10, 2022

Uh oh!

pull-bot commented Nov 11, 2022

Uh oh!

gabrieldonadel commented Nov 12, 2022

Uh oh!

Uh oh!

necolasNov 14, 2022

Choose a reason for hiding this comment

Uh oh!

gabrieldonadelNov 17, 2022

Choose a reason for hiding this comment

Uh oh!

pull-bot commented Nov 16, 2022

Uh oh!

pull-bot commented Nov 17, 2022

Uh oh!

Uh oh!

pull-bot commented Nov 17, 2022

Uh oh!

gabrieldonadel commented Nov 17, 2022

Uh oh!

facebook-github-bot commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

analysis-bot commented Nov 4, 2022•
edited
Loading

analysis-bot commented Nov 4, 2022•
edited
Loading

necolasNov 4, 2022•
edited
Loading