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.

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

Enable event loop by default when bridgeless is enabled #43396

Closed
wants to merge 2 commits into from
Closed

Enable event loop by default when bridgeless is enabled #43396

wants to merge 2 commits into from

Conversation

rubennorte
Copy link
Contributor

@rubennorte rubennorte commented Mar 8, 2024 •
edited
Loading

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Fixes #42742 and probably #33006.

Differential Revision: D54682678

@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 Mar 8, 2024
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 11, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
c863eed 
Summary:
Pull Request resolved: facebook#43396

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Differential Revision: D54682678
@analysis-bot
Copy link

analysis-bot commented Mar 11, 2024 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 18,125,846 -1,248
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,495,385 +1,560
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: e180f80
Branch: main

@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 12, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
9082cb8 
Summary:
Pull Request resolved: facebook#43396

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 12, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
0ef76b5 
Summary:
Pull Request resolved: facebook#43396

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 12, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
5ff82d4 
Summary:
Pull Request resolved: facebook#43396

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 13, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
e14b957 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 13, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
8ea1563 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 15, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
2e7ae6f 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 15, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
f9cd095 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 15, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
3f50f19 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@facebook-github-bot
Copy link
Contributor

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

@rubennorte @facebook-github-bot
Update OSS renderers with compatibility with event loop (facebook#43507)
7a1e958 
Summary:

Changelog: [General][Changed] - sync React renderers to 18.3.0-canary-9372c6311-20240315

Syncs React renderers to facebook/react@9372c63 which is canary for 18.3.0-canary-9372c6311-20240315.

This includes the necessary changes to enable the use of microtasks for scheduling in Fabric.

Differential Revision: D54947212
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
1ed4b83 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
rubennorte added a commit to rubennorte/react-native that referenced this pull request Mar 15, 2024
@rubennorte @facebook-github-bot
Enable event loop by default when bridgeless is enabled (facebook#43396)
eaa0964 
Summary:

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678
@rubennorte rubennorte force-pushed the export-D54682678 branch 2 times, most recently from eaa0964 to 1ed4b83 Compare March 15, 2024 18:11
@facebook-github-bot
Copy link
Contributor

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

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

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 18, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 87b73a0.

@rubennorte rubennorte mentioned this pull request Mar 18, 2024
Closed
@rubennorte rubennorte deleted the export-D54682678 branch March 18, 2024 12:07
cipolleschi pushed a commit that referenced this pull request Apr 30, 2024
@rubennorte @cipolleschi
Enable event loop by default when bridgeless is enabled (#43396)
b440672 
Summary:
Pull Request resolved: #43396

Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.

Reviewed By: rshest

Differential Revision: D54682678

fbshipit-source-id: ff8c45bc1caae0e9182aa94d915d7b6f9427caf9
@atn4z7 atn4z7 mentioned this pull request May 8, 2024
Merged
4 tasks
@kraenhansen kraenhansen mentioned this pull request Jun 18, 2024
Merged
7 tasks
@kraenhansen
Copy link
Contributor

I tried disabling our workaround for #33006 based on the value of the ReactNativeFeatureFlags.enableMicrotasks() and unfortunately it doesn't seem to solve it.

I wonder if it'd be possible to extend rn-tester's NativeCxxModuleExample to exhibit this behaviour 🤔

@rubennorte
Copy link
Contributor Author

I tried disabling our workaround for #33006 based on the value of the ReactNativeFeatureFlags.enableMicrotasks() and unfortunately it doesn't seem to solve it.

I wonder if it'd be possible to extend rn-tester's NativeCxxModuleExample to exhibit this behaviour 🤔

Could you please open a new issue with the details about how to reproduce this with the new architecture enabled? (that enables the new event loop by default).

I took a quick look at the example in https://github.com/mfbx9da4/react-native-jsi-promise/blob/main/ios/JsiPromise.mm and I still see problems about how those promises are resolved (you can't call into JSI from any threads/queues, you should use the JS thread or you'd get undefined behaviors). To call into JS, you should use things like the call invoker that's passed to C++ TurboModules, which allows you to schedule async jobs in the JS thread, where you can safely resolve the promise.

@kraenhansen
Copy link
Contributor

kraenhansen commented Jun 19, 2024 •
edited
Loading

Could you please open a new issue with the details about how to reproduce this with the new architecture enabled?

I agree, that's probably the best next step, which was why I mentioned extending rn-tester. I mainly wanted to mention that I failed to verify the fix in our code and I'll have to circle back to crafting a minimal reproduction of this later. I'll create a new issue referencing this PR and the original issue and tagging you, once I eventually get there 👍

you should use the JS thread or you'd get undefined behaviors

We're asserting the thread is the JS thread in our implementation (I'm not associated with the example you posted).

@kraenhansen kraenhansen mentioned this pull request Jun 28, 2024
Closed
This was referenced Jun 28, 2024
Closed
Merged
@kraenhansen
Copy link
Contributor

Please disregard my comments above .. As I outline in my latest comment on #33006 our way of scheduling async work was not through the CallInvoker and therefore didn't trigger drainage of the microtask queue.

@cipolleschi cipolleschi mentioned this pull request Jul 22, 2024
Merged
@cipolleschi cipolleschi mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hermes WeakRef Support in React Native
4 participants
@rubennorte @facebook-github-bot @analysis-bot @kraenhansen