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

Enable RuntimeScheduler in old architecture #37523

Closed
wants to merge 1 commit into from

Conversation

sammy-SC
Copy link
Contributor

@sammy-SC sammy-SC commented May 22, 2023

Summary:

Changelog:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Test Plan:

Run RNTester with old architecture and make sure RuntimeScheduler is used.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels May 22, 2023
@analysis-bot
Copy link

analysis-bot commented May 22, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,739,234 -922
android hermes armeabi-v7a 8,050,425 -839
android hermes x86 9,229,413 -951
android hermes x86_64 9,081,028 -967
android jsc arm64-v8a 9,301,178 -915
android jsc armeabi-v7a 8,489,951 -839
android jsc x86 9,362,429 -949
android jsc x86_64 9,618,151 -973

Base commit: 1a1e399
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

sammy-SC added a commit that referenced this pull request May 22, 2023
Summary:
## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #37523

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: adbe9254fc4a55f7a6a5540ca0e44f26c7ccb45c
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 78885a3 to de290e0 Compare May 22, 2023 20:37
@github-actions
Copy link

This pull request was successfully merged by @sammy-SC in de290e0.

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

@github-actions github-actions bot added the Merged This PR has been merged. label May 22, 2023
sammy-SC added a commit that referenced this pull request May 22, 2023
Summary:
## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #37523

Test Plan: Run RNTester with Paper and make sure native scheduler is used.

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 0ba3d3096639986734a433ec8aae8564c70a2e66
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from de290e0 to 19c2596 Compare May 22, 2023 21:41
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

sammy-SC added a commit that referenced this pull request May 22, 2023
Summary:
## Changelog:
[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with Paper and make sure native scheduler is used.

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 1790b4f42c0df428e26da482d0c895c20741cdbd
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 19c2596 to 9481858 Compare May 22, 2023 22:08
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

sammy-SC added a commit that referenced this pull request May 23, 2023
Summary:
## Changelog:
[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with Paper and make sure native scheduler is used.

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 6cee070312034c5606799049a798b3b5659d03f3
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch 2 times, most recently from f5e26cd to f59723e Compare May 23, 2023 10:27
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

sammy-SC added a commit that referenced this pull request May 25, 2023
Summary:
## Changelog:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Reviewed By: cipolleschi

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 2448f9578114dba9c4c855acd1f61e7468cd8f37
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 98acec6 to 8927517 Compare May 25, 2023 11:25
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

sammy-SC added a commit that referenced this pull request May 25, 2023
Summary:
## Changelog:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Reviewed By: cipolleschi

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 89095e8aede025bbeaeb02196ee52de301c8fd05
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 8927517 to 7a0868a Compare May 25, 2023 11:41
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

sammy-SC added a commit that referenced this pull request May 25, 2023
Summary:
## Changelog:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 98cdb0b588f4e30e4585f3f09d8633dfc2bb4e70
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 7a0868a to 1342130 Compare May 25, 2023 12:36
Summary:
## Changelog:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 6b3dccb9aaf4e3e3beaf723098359a83be23ae5c
@sammy-SC sammy-SC force-pushed the sammy/enable-runtimescheduler-for-old-arch branch from 1342130 to 0c10fc8 Compare May 25, 2023 13:30
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D46078324

cipolleschi pushed a commit that referenced this pull request May 25, 2023
Summary:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: #37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Reviewed By: cipolleschi

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 5f18cbe60e6c9c753c373f175ba413b79288a928
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 88eef42.

Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request May 31, 2023
Summary:

[IOS] [FIXED] - unexpected useEffects flushing semantics

Pull Request resolved: facebook#37523

Test Plan: Run RNTester with old architecture and make sure RuntimeScheduler is used.

Reviewed By: cipolleschi

Differential Revision: D46078324

Pulled By: sammy-SC

fbshipit-source-id: 5f18cbe60e6c9c753c373f175ba413b79288a928
tomekzaw added a commit to software-mansion/react-native-reanimated that referenced this pull request Jun 7, 2023
## Summary

This PR bumps react-native to 0.72.0-rc.5 and slightly changes
initialization path on iOS (Paper) due to recent changes in the
framework (facebook/react-native#37523).

Fixes #4521. Continues #3005.

On iOS, Reanimated needs to overwrite two React internal modules:
* `RCTUIManager` &rarr; `REAUIManager` to intercept `manageChildren`
calls in order to observe React tree changes
([here](https://github.com/software-mansion/react-native-reanimated/blob/main/ios/LayoutReanimation/REAUIManager.mm))
* `RCTEventDispatcher` &rarr; `REAEventDispatcher` to intercept events
in the native code while still on the UI thread
([here](https://github.com/software-mansion/react-native-reanimated/blob/663ee74925cafa4a1a802f4722466dc02cb1760f/ios/REAEventDispatcher.m#L9-L10))

The rest of the initialization (injecting JSI bindings) can be safely
done in `installTurboModule` method, as we already do on Android and
iOS/Fabric.

Previously, this was done using categories (see
[`UIResponder+Reanimated.mm`](https://github.com/software-mansion/react-native-reanimated/blob/663ee74925cafa4a1a802f4722466dc02cb1760f/ios/native/UIResponder%2BReanimated.mm#L19-L30)),
in particular by overwriting `jsExecutorFactoryForBridge:` method which
is called during initialization. However, since 0.72.0-rc.4, this method
is already implemented in `RCTAppDelegate` so the trick does no longer
work, making the app fail with the following error "[Reanimated] The
native part of Reanimated doesn't seem to be initialized".

In this PR, I've used a category to overwrite another method
`extraModulesForBridge` which swizzles the implementation of
`jsExecutorFactoryForBridge` method and runs `REAInitializer` before the
original call.

As suggested by @kmagiera, alternatively we could just swizzle the
methods of `RCTUIManager` and `RCTEventDispatcher`.

## TODO

- [x] Restore support for 0.71 and below with #ifdefs

## Test plan

<!-- 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. -->
@cortinico cortinico deleted the sammy/enable-runtimescheduler-for-old-arch branch June 10, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. p: Facebook Partner: Facebook Partner Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants