[ios][expotools] Fixes for versioned TurboModules #9862
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wkozyra95
suggested changes
Aug 21, 2020
There was a problem hiding this comment.
Originally I misunderstand a bit what you wanted to do, change I suggested will still require a modifications in react-native but at least the lifecycle of object will be preserved.
In this PR I see pointer converted to void*, but I don't see anywhere that would restore it, am I missing sth?
|
I updated my suggestion after looking into react-native changes |
bbarthec
approved these changes
Aug 21, 2020
sjchmiela
force-pushed
the
@sjchmiela/turbomodules-fixes
branch
2 times, most recently
from
4253e79
to
3e837ad
Compare
tsapeta
approved these changes
Aug 25, 2020
wkozyra95
approved these changes
Aug 25, 2020
sjchmiela
force-pushed
the
@sjchmiela/turbomodules-fixes
branch
from
3e837ad
to
acb85e4
Compare
bbarthec
pushed a commit
that referenced
this pull request
Aug 25, 2020
# Why After versioning, TurboModules integration started to make more harm than good. # How - moved creating of the `JSCExecutorFactory`, which should be versioned, to versioned realm (otherwise we try to pass an unversioned code into a versioned bridge) (huge thanks to C++ senpai @wkozyra95 for assistance) - to achieve this I unfortunately had to modify `react-native` to 1. expect `void *` from `jsExecutorFactoryForBridge:`, otherwise we can't pass versioned factory through unversioned and back into versioned 2. do not detect whether delegate is an instance of `RCTCxxBridgeDelegate` by protocol, but by the actual method used. This is because delegate for all bridges is always `EXReactAppManager` which is not versioned. - another change is that `EXVersionManager` now **holds strongly** one instance of the `JSCExecutorFactory` instead of just creating it on every call. I think this shouldn't break anything, as on each bridge reload we create a new instance of `EXVersionManager` and there shouldn't be more calls to `jsExecutorFactoryForBridge:` apart from when setting the bridge up, but I think it's a note-worthy mention here. - then I bumped into the `XX is not a registered callable module` error which led me to update `expotools`'s pattern for adding `EX_REMOVE_VERSION` to `MessageQueue`. # Test Plan I have verified that these fixes work on a separate branch.
This was referenced Sep 15, 2021
This was referenced Sep 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
After versioning, TurboModules integration started to make more harm than good.
How
JSCExecutorFactory, which should be versioned, to versioned realm (otherwise we try to pass an unversioned code into a versioned bridge) (huge thanks to C++ senpai @wkozyra95 for assistance)react-nativetovoid *fromjsExecutorFactoryForBridge:, otherwise we can't pass versioned factory through unversioned and back into versionedRCTCxxBridgeDelegateby protocol, but by the actual method used. This is because delegate for all bridges is alwaysEXReactAppManagerwhich is not versioned.EXVersionManagernow holds strongly one instance of theJSCExecutorFactoryinstead of just creating it on every call. I think this shouldn't break anything, as on each bridge reload we create a new instance ofEXVersionManagerand there shouldn't be more calls tojsExecutorFactoryForBridge:apart from when setting the bridge up, but I think it's a note-worthy mention here.XX is not a registered callable moduleerror which led me to updateexpotools's pattern for addingEX_REMOVE_VERSIONtoMessageQueue.Test Plan
I have verified that these fixes work on a separate branch.