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

[Animated] Regression runtime error if passing undefined to Animated.View style #2523

Closed
1 task done
Kudo opened this issue May 22, 2023 · 6 comments
Closed
1 task done
Labels
Milestone

Comments

@Kudo
Copy link

Kudo commented May 22, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the issue

If passing undefined to Animated.View style, there will be a runtime error. This issue does not happen on react-native-web 0.18. I think this should be a regression issue.

Example code:

function App() {
  return (
    <View style={{ flex: 1 }}>
      <Animated.View style={{ height: 64, transform: undefined }}>
        <View style={{ width: 50, height: 50, backgroundColor: "green" }} />
      </Animated.View>
    </View>
  );
}

Error stacktrace:

AnimatedTransform.js:62 Uncaught TypeError: Cannot read properties of undefined (reading 'forEach')
    at AnimatedTransform.__attach (AnimatedTransform.js:62:1)
    at AnimatedTransform.__addChild (AnimatedWithChildren.js:34:1)
    at AnimatedStyle.__attach (AnimatedStyle.js:86:1)
    at AnimatedStyle.__addChild (AnimatedWithChildren.js:34:1)
    at AnimatedProps.__attach (AnimatedProps.js:63:1)
    at new AnimatedProps (AnimatedProps.js:29:1)
    at useAnimatedProps.js:29:1
    at mountMemo (react-dom.development.js:17225:1)
    at Object.useMemo (react-dom.development.js:17670:1)
    at useMemo (react.development.js:1650:1)

Expected behavior

the undefined style should not raise runtime error.

Steps to reproduce

I have a minimal reproducible example created from create-react-app

$ git clone https://github.com/Kudo/repro-rn-web-animated-transform.git
$ cd repro-rn-web-animated-transform
$ npm i
$ npm start

Test case

https://github.com/Kudo/repro-rn-web-animated-transform/

Additional comments

No response

@Kudo Kudo added the bug label May 22, 2023
@necolas necolas added this to the 0.19.x milestone May 22, 2023
@necolas necolas changed the title Regression runtime error if passing undefined to Animated.View style [Animated] Regression runtime error if passing undefined to Animated.View style May 31, 2023
Kudo added a commit to expo/expo that referenced this issue Jun 7, 2023
# Why

upgrade react-native 0.72 for sdk 49
close ENG-8011

# How

- bump package versions
  - `react-native 0.71.3 -> 0.72.0-rc.5`
  - `metro-react-native-babel-preset 0.73.9 -> 0.76.5`
- [bare-expo][templates][fabric-tester] migrate template base on [upgrade-helper](https://react-native-community.github.io/upgrade-helper/?from=0.71.7&to=0.72.0-rc.5)
- [expo-template-tabs] remove the metro version overrides for expo-router.
- [core][dev-laucher][dev-menu][media-library][screen-orientation][splash-screen][updates-interface][updates] add the `install_modules_dependencies` to support new architecture + use_frameworks!
- [core][autolinking] fix some new architecture error on ios
- [react-native-lab] update our fork to 0.72.0-rc.5 based
- [go][tools] fix **react-native-lab/react-native/packages/react-native** path move because of react-native's repo monorepo changes
- [go][android] fix gradle 8 errors
- [go][ios] add `RCT_REMOTE_PROFILE=0` to fix the `RCT_ENABLE_INSPECTOR needs to be set to fulfill RCT_REMOTE_PROFILE` build error
- [ncl] remove `ProgressViewIOS` / `ProgressBarAndroid` since they are deprecated/removed in 0.72
- [dev-menu][dev-launcher] rebuild bundles

# Note

- react-native-web is not bumped because of the [issue](necolas/react-native-web#2523), so it's still react-native-web@~0.18.10.
- currently disable ci typecheck for @expo/cli because of upstream metro typescript support. i'll have another pr to fix those errors.
- updates e2e ci on android is broken at [here](https://github.com/expo/expo/blob/fada3d764957779fbfc3d7b723d185db1d933d95/packages/expo-updates/e2e/fixtures/Updates.e2e.ts#L518). i doubt if that's related to the react scheduler change. i'd disabled the failed test case.
- the react-native upstream [migrated away the `@types/jest`](facebook/react-native#36068). i was afraid that will be a breaking change to the existing jest test code since it requires the explicit `@jest/globals` import. i didn't do this in this upgrade.

# Test Plan

- ✅ fabric-tester (without expo-dev-client)
- ✅ ci passed. there are some errors which are known:
  - updates e2e on android: as mentioned above
  - ios expo go on eas build: versioned expo go are broken on eas build m1 worker. this is also happening on main.
  - android client: no space left on the ubuntu worker. this is also happening on main.
- ✅ bare-expo
- ✅ unversioned expo go + ncl

---------

Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com>
@necolas
Copy link
Owner

necolas commented Jun 27, 2023

In the future, please create test cases using the link provided in the issue template. It saves me a lot of time and inconvenience. Creating a separate repo isn't helpful

@Kudo
Copy link
Author

Kudo commented Jun 27, 2023

sorry for the inconvenience. i did follow the issue template. in the "Test case" field, does it require a codesandbox rather than a separate repo?

  - type: input
    attributes:
      label: Test case
      description: Please provide a link to a reduced test case that reproduces the issue.
      placeholder: "https://codesandbox.io/s/6lx6ql1w5r"
    validations:
      required: true

@necolas
Copy link
Owner

necolas commented Jun 27, 2023

It's ok. There's a test case for the same issue in #2528

@Kudo
Copy link
Author

Kudo commented Jun 27, 2023

cool, using codesandbox makes the repro much easier. will try to do that next time. thanks!

necolas added a commit that referenced this issue Jun 27, 2023
@dan-doyon-endear
Copy link

dan-doyon-endear commented Jun 28, 2023

I don't do PRs but here is a patch that works for us.

diff --git a/node_modules/react-native-web/dist/vendor/react-native/Animated/nodes/AnimatedTransform.js b/node_modules/react-native-web/dist/vendor/react-native/Animated/nodes/AnimatedTransform.js
index 66072d9..b92ada1 100644
--- a/node_modules/react-native-web/dist/vendor/react-native/Animated/nodes/AnimatedTransform.js
+++ b/node_modules/react-native-web/dist/vendor/react-native/Animated/nodes/AnimatedTransform.js
@@ -16,7 +16,7 @@ import NativeAnimatedHelper from '../NativeAnimatedHelper';
 class AnimatedTransform extends AnimatedWithChildren {
   constructor(transforms) {
     super();
-    this._transforms = transforms;
+    this._transforms = transforms || [];
   }
   __makeNative() {
     this._transforms.forEach(transform => {

@Kudo
Copy link
Author

Kudo commented Jun 29, 2023

much appreciated for the fix and releasing 0.19.6, thanks @necolas!

Kudo added a commit to expo/expo that referenced this issue Jun 30, 2023
# Why

bump react-native-web to 0.19.6 for sdk 49

# How

- since [the issue](necolas/react-native-web#2523) is fixed by react-native-web@0.19.6. it's a good time to bump the version.
- [ncl] fix some broken imports
- [ncl] fix gesture-handler error 

# Test Plan

- smoke testing ncl on web
- ci passed
Kudo added a commit to expo/expo that referenced this issue Jun 30, 2023
# Why

bump react-native-web to 0.19.6 for sdk 49

# How

- since [the issue](necolas/react-native-web#2523) is fixed by react-native-web@0.19.6. it's a good time to bump the version.
- [ncl] fix some broken imports
- [ncl] fix gesture-handler error

# Test Plan

- smoke testing ncl on web
- ci passed

(cherry picked from commit f0e814a)
nahn20 pushed a commit to nahn20/expo-media-library that referenced this issue Mar 4, 2024
# Why

upgrade react-native 0.72 for sdk 49
close ENG-8011

# How

- bump package versions
  - `react-native 0.71.3 -> 0.72.0-rc.5`
  - `metro-react-native-babel-preset 0.73.9 -> 0.76.5`
- [bare-expo][templates][fabric-tester] migrate template base on [upgrade-helper](https://react-native-community.github.io/upgrade-helper/?from=0.71.7&to=0.72.0-rc.5)
- [expo-template-tabs] remove the metro version overrides for expo-router.
- [core][dev-laucher][dev-menu][media-library][screen-orientation][splash-screen][updates-interface][updates] add the `install_modules_dependencies` to support new architecture + use_frameworks!
- [core][autolinking] fix some new architecture error on ios
- [react-native-lab] update our fork to 0.72.0-rc.5 based
- [go][tools] fix **react-native-lab/react-native/packages/react-native** path move because of react-native's repo monorepo changes
- [go][android] fix gradle 8 errors
- [go][ios] add `RCT_REMOTE_PROFILE=0` to fix the `RCT_ENABLE_INSPECTOR needs to be set to fulfill RCT_REMOTE_PROFILE` build error
- [ncl] remove `ProgressViewIOS` / `ProgressBarAndroid` since they are deprecated/removed in 0.72
- [dev-menu][dev-launcher] rebuild bundles

# Note

- react-native-web is not bumped because of the [issue](necolas/react-native-web#2523), so it's still react-native-web@~0.18.10.
- currently disable ci typecheck for @expo/cli because of upstream metro typescript support. i'll have another pr to fix those errors.
- updates e2e ci on android is broken at [here](https://github.com/expo/expo/blob/fada3d764957779fbfc3d7b723d185db1d933d95/packages/expo-updates/e2e/fixtures/Updates.e2e.ts#L518). i doubt if that's related to the react scheduler change. i'd disabled the failed test case.
- the react-native upstream [migrated away the `@types/jest`](facebook/react-native#36068). i was afraid that will be a breaking change to the existing jest test code since it requires the explicit `@jest/globals` import. i didn't do this in this upgrade.

# Test Plan

- ✅ fabric-tester (without expo-dev-client)
- ✅ ci passed. there are some errors which are known:
  - updates e2e on android: as mentioned above
  - ios expo go on eas build: versioned expo go are broken on eas build m1 worker. this is also happening on main.
  - android client: no space left on the ubuntu worker. this is also happening on main.
- ✅ bare-expo
- ✅ unversioned expo go + ncl

---------

Co-authored-by: Tomasz Sapeta <tomasz.sapeta@swmansion.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants