-
Notifications
You must be signed in to change notification settings - Fork 25k
Enable event loop by default when bridgeless is enabled #43396
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
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Pull Request resolved: facebook#43396 Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Differential Revision: D54682678
8d4c8a3 to
c863eed
Compare
Base commit: e180f80 |
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Pull Request resolved: facebook#43396 Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Differential Revision: D54682678
c863eed to
9082cb8
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Pull Request resolved: facebook#43396 Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Differential Revision: D54682678
9082cb8 to
0ef76b5
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
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
0ef76b5 to
5ff82d4
Compare
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
5ff82d4 to
e14b957
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
e14b957 to
8ea1563
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
8ea1563 to
2e7ae6f
Compare
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
2e7ae6f to
f9cd095
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
b3f8274 to
3f50f19
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
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
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
Summary: Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled. Reviewed By: rshest Differential Revision: D54682678
eaa0964 to
1ed4b83
Compare
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D54682678 |
|
This pull request has been merged in 87b73a0. |
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
|
I tried disabling our workaround for #33006 based on the value of the 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. |
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 👍
We're asserting the thread is the JS thread in our implementation (I'm not associated with the example you posted). |
|
Please disregard my comments above .. As I outline in my latest comment on #33006 our way of scheduling async work was not through the |
Changelog: [General][Changed] Enabled new event loop behavior when bridgeless (new architecture) is enabled.
Fixes #42742 and probably #33006.
Differential Revision: D54682678