Skip to content

Fix getAnimatedStyle error when called with component that doesn't have animated styles #6746

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Feb 24, 2025
Merged

Fix getAnimatedStyle error when called with component that doesn't have animated styles #6746

merged 12 commits into from
Feb 24, 2025

Conversation

AcostaB
Copy link
Contributor

@AcostaB AcostaB commented Nov 21, 2024

isEmpty will return false if it is passed in undefined, instead of throwing an error.

Summary

If getAnimatedStyle is passed in a component that doesn't have animated styles, it throws an error saying "Cannot convert undefined or null to object". This happens because jestUtils.ts has an isEmpty function that calls Object.keys with an argument that can potentially be undefined. The arg can potentially be undefined because it comes from const jestAnimatedStyleValue = component.props.jestAnimatedStyle?.value;

This issue was introduced in version 3.16.0.

Test plan

Can be simply tested on a browser console. Just run:

(obj => Object.keys(obj).length === 0)(undefined) // throws error

(obj => !obj || Object.keys(obj).length === 0)(undefined) // returns true as expected

(obj => !obj || Object.keys(obj).length === 0)({test: 'something'}) // returns false as expected

bartlomiejbloniarz and others added 7 commits October 16, 2024 18:02
@bartlomiejbloniarz
Fix CallInvokerHolderImpl backwards compatibility (software-mansion…
1e6ed83 
…#6601)

## Summary
This PR changes the way `CallInvokerHandlerImpl` is obtained on older
versions of RN. It seems that there is a bug in our build checking CI,
since calling `context.getJSCallInvokerHolder()` is not valid on older
versions of RN.

## Test plan
@tomekzaw
Move headers to apple directory (software-mansion#6612)
7d14cea 
<!-- Thanks for submitting a pull request! We appreciate you spending
the time to work on these changes. Please follow the template so that
the reviewers can easily understand what the code changes affect. -->

This PR moves iOS/macOS/tvOS headers from `apple/reanimated/` to yet
another `apple` subdirectory so that the file tree reflects the paths in
`#import <...>`.

After this change, `apple/reanimated` can be added to
`HEADER_SEARCH_PATHS` and imports like
`reanimated/sensor/ReanimatedSensor.h` will work properly.

<!-- Provide a minimal but complete code snippet that can be used to
test out this change along with instructions how to run it and a
description of the expected behavior. -->
@tomekzaw
Remove config[:reanimated_node_modules_dir] in reanimated_utils.rb (s…
969bad6 
…oftware-mansion#6611)

## Summary

This PR removes `config[:reanimated_node_modules_dir]` because it looks
like it's unused (at least I hope so).

Apart from this, the value is incorrect (it's
`/Users/tomekzaw/RNOS/react-native-reanimated/packages` on my setup but
should be
`/Users/tomekzaw/RNOS/react-native-reanimated/packages/react-native-reanimated`).

## Test plan
@tomekzaw
Fix static linking on iOS (software-mansion#6614)
3bf4cd8 
Fixes
software-mansion#6607.

This is more of a workaround to fix issue with missing headers when
building Reanimated with static linkage for iOS. I spent several hours
on trying to make it right but it would work correctly only partially. I
will post my findings here or internally later on and I hope to find a
better solution sometime in the future.

This workaround adds paths to `Common/cpp` and `apple/` directories to
`HEADER_SEARCH_PATHS` – the preprocessor looks for headers in these
locations. I can't use an absolute path (calculated in Ruby) because it
would make the output setup-dependent and thus affect checksums in
`Podfile.lock` which is unwanted as some apps check against checksum
changes on CI.

Tested on fabric-example app with `USE_FRAMEWORKS=static` and without.

---

When installing pods in `fabric-example` with `cd ios && bundle install
&& bundle exec pod install`, the headers are visible in `ios/Pods`
directory:

```
$ find . -name "REAUIKit.h"
./Pods/Headers/Public/RNReanimated/reanimated/apple/REAUIKit.h
./Pods/Headers/Private/RNReanimated/reanimated/apple/REAUIKit.h
```

However, after running `USE_FRAMEWORKS=static bundle exec pod install`,
the headers are no longer present:

```
$ find . -name "REAUIKit.h"
<no output>
```

Because of this, `#import <reanimated/apple/REAUIKit.h>` doesn't work,
but when changed back to `#import <RNReanimated/REAUIKit.h>` it works
fine (jump to file also works in Xcode).

This made me wonder what's the location of `REAUIKit.h`. Obviously, the
file is located in
`react-native-reanimated/packages/react-native-reanimated/apple/reanimated/apple/REAUIKit.h`
(using symlinks as a Development Pod), but the path is not included in
header search paths. Hence, I decided to add
`react-native-reanimated/packages/react-native-reanimated/apple/` to
header search paths. However, this can't be done using absolute paths
because they are likely to be machine-specific (e.g. containing the home
directory name) which also affects checksum in Podfile.lock which is
inconvenient since some setups assume the checksum to be stable (unless
the version of the library is changed) due to security concerns.

A better idea would be to take inspiration from react-native itself.
I've noticed that `ReactCommon` has a similar feature – the headers are
in nested subdirectories (e.g. `react/renderer/core/ShadowNode.h`) and
the imports don't assume flat structure (e.g. it's `#import
<react/renderer/core/ShadowNode.h>` instead of `#import
<ReactCommon/ShadowNode.h>`).

I remembered that frameworks create `FrameworkName.framework`
directories. I finally found `RNReanimated.framework` in Xcode build
folder that you can open from menu bar:

<img width="312" alt="Screenshot 2024-10-18 at 18 04 45"
src="https://github.com/user-attachments/assets/1db95b44-d21c-4e9a-a1d5-19674093094b">

Taking a quick look inside and comparing `ReactCommon` and
`RNReanimated`, I've noticed that `RNReanimated.framework` doesn't
contain any header files (except for umbrella header) while
`ReactCommon` does:

<img width="1178" alt="Screenshot 2024-10-18 at 18 05 33"
src="https://github.com/user-attachments/assets/2633ab64-a85e-4a04-b2b3-97a11e576d6c">

When I commented out all 3 occurrences of `header_mappings_dir` in
RNReanimated.podspec and run the Xcode build (first build lasts until
the first error, if you hit the play button once again then, I assume it
runs the remaining tasks), the headers were finally there but the
directory structure was flattened:

```diff
-ss.header_mappings_dir = "Common/cpp/reanimated"
+#ss.header_mappings_dir = "Common/cpp/reanimated"
```

<img width="1178" alt="Screenshot 2024-10-18 at 18 14 52"
src="https://github.com/user-attachments/assets/c25c1dfe-ce45-45ff-856f-be005907ffe2">

Then I tried passing an absolute path to appropriate directories to see
if this would fix the structure:

```diff
-ss.header_mappings_dir = "Common/cpp/reanimated"
+ss.header_mappings_dir = "/Users/tomekzaw/RNOS/react-native-reanimated/packages/react-native-reanimated/Common/cpp"

-sss.header_mappings_dir = "apple/reanimated"
+sss.header_mappings_dir = "/Users/tomekzaw/RNOS/react-native-reanimated/packages/react-native-reanimated/

-ss.header_mappings_dir = "Common/cpp/worklets"
+ss.header_mappings_dir = "/Users/tomekzaw/RNOS/react-native-reanimated/packages/react-native-reanimated/Common/cpp"
```

Then I investigated `ReactCommon.podspec` inside react-native repository
and found several interesting lines of code:

```rb
s.header_dir = "ReactCommon" # Use global header_dir for all subspecs for use_frameworks! compatibility
```

It looks like `header_dir` must contain `header_mappings_dir` for the
latter to work properly when `use_frameworks!` is enabled.

```rb
if ENV['USE_FRAMEWORKS']
  s.header_mappings_dir     = './'
end
```
For some reason, `s.header_mappings_dir` is set to the current directory
only if `USE_FRAMEWORKS` is set.

...

```
HEADER_SEARCH_PATHS = (
	"$(inherited)",
	"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers",
	"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers/react/nativemodule/core",
	"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon-Samples/ReactCommon_Samples.framework/Headers",
	"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon-Samples/ReactCommon_Samples.framework/Headers/platform/ios",
	"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/components/view/platform/cxx",
	"${PODS_CONFIGURATION_BUILD_DIR}/React-NativeModulesApple/React_NativeModulesApple.framework/Headers",
	"${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers",
	"${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers/react/renderer/graphics/platform/ios",
);
```
@tomekzaw
Bump reanimated version to 3.16.1
2dd6b42
@tjzel @WoLewicki
release: 3.16.2 (software-mansion#6735)
94ca94a 
## Summary

Cherry-picking:

- software-mansion#6715
- software-mansion#6705
- software-mansion#6706
- software-mansion#6725
- software-mansion#6667

## Test plan

---------

Co-authored-by: Wojciech Lewicki <[email protected]>
@AcostaB
Update jestUtils.ts - isEmpty now handles undefined arg
ae3d7a2 
isEmpty will return false if it is passed in undefined
piaskowyk
piaskowyk approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@piaskowyk piaskowyk 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 your changes! 🙌 I'll merge them into the main branch and backport them to version 3.17.

@piaskowyk piaskowyk changed the base branch from 3.16-stable to main February 24, 2025 09:31
@piaskowyk piaskowyk added this pull request to the merge queue Feb 24, 2025
Merged via the queue into software-mansion:main with commit ce4f056 Feb 24, 2025
9 checks passed
@piaskowyk piaskowyk mentioned this pull request Feb 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piaskowyk piaskowyk piaskowyk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AcostaB @piaskowyk @bartlomiejbloniarz @tomekzaw @tjzel