-
Notifications
You must be signed in to change notification settings - Fork 25.1k
feat(iOS): early TurboModule discovery #53277
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
deb7658 to
b1984c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I left a few comments that has to be addressed.
This feature might also create some performance issues, delaying the app startup.
It would be great if you can wrap the _discoverModules in a featureFlag check, so we can disable it if we have a regression in the startup performance.
Feature flags for React Native are documented here
|
|
||
| @protocol RCTTurboModuleManagerDelegate <NSObject> | ||
|
|
||
| - (nonnull NSArray<NSString *> *)getModuleNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a breaking change. every implementer of the delegate will have to implement this.
It is better to move this to be optional and to handle the optionality.
Also, the documentation is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know there is an application for multiple delegate implementers. I'll make it optional as you suggest and add relevant documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e3db6d5.
| #pragma mark - Private Methods | ||
|
|
||
| - (void)_discoverModules{ | ||
| NSArray<NSString *> *moduleNames = [_delegate getModuleNames]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, make the method in the delegate @optional, and handle this here like:
if ([_delegate respondsToSelector:@selector(getModuleNames)]) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e3db6d5.
| #pragma mark - RCTTurboModuleManagerDelegate | ||
|
|
||
| - (nonnull NSArray<NSString *> *)getModuleNames{ | ||
| return [_delegate getModuleNames]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check whether the delegate implements this method or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e3db6d5.
| } | ||
|
|
||
| - (nonnull NSArray<NSString *> *)getModuleNames{ | ||
| return [dependencyProvider moduleNames]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check whether the dependencyProvider implement this method.
People could, in principle, create custom dependency providers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e3db6d5.
|
|
Thanks for the review! I tested the changes flipping the feature flag on and off and it worked as expected. |
|
@tjzel, could you elaborate on the motivation behind this change? Why do we need the module holders initialized early? |
|
@RSNara I talked about it in this comment #53282 (comment), early module holder initialization was for the purpose of finding modules conforming to a certain protocol. |
|
Closing in favor of #53928 |
Summary:
At the moment user-made TurboModules are discovered during the evaluation of the script - conversely to Android, where it happens before this evaluation. Because of that it's impossible to instantiate a module that we don't know the name of pre-bundle.
To implement this I used
modulesProvidercodegen config field to get the names of the TurboModules and initialize their holders early.Changelog:
[IOS] [ADDED] - Added ability to discover TurboModules pre-bundle.
Test Plan: