-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
fix: add custom request handlers on new arch #36071
fix: add custom request handlers on new arch #36071
Conversation
Base commit: 34604fa |
[modules addObjectsFromArray:[bridge modulesConformingToProtocol:protocol]]; | ||
} | ||
|
||
// TODO: find a way to get the same information of turboModules |
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 think that the problem here is that all the TM are lazily loaded. The system does not know at startup which modules are TM and which are not. So, it's not possible to query the TurboModule registry to check which modules have been registered.
One way could be to be explicit and provide a method to list the handlers that the user has to install. However this will pollute the user space that now has suddenly to do an extra thing to properly configure its app.
Another way could be to rely on Codegen, we can introduce a default class that is generated which provides the URLRequestHandlers for the turbomodules that are marked is some specific way.
Alternatively, we could try to see what the CLI does when it autolinks and add a new step there.
This is probably the best approach as we could make it efficient and transparent.
What do you think?
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 think both approaches sound good and I don't have enough knowledge of the internals to be able to say more 😅 Anything that works and does not require the user of the library to do anything is ok for me.
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 think there shouldn't be a need to introduce this modulesConformingToProtocol API in the TurboModule system for this use-case. The application knows exactly which modules are registered with it (including all the ones it gets from libraries). So, the application should be able to create and return an RCTNetworking module with all the RCTURLRequestHandlers in that application.
The work is connected with bringing new arch to react-native-cameraroll: react-native-cameraroll/react-native-cameraroll#460
@WoLewicki, could you describe your problem in more details? Is the problem that React Native Camera Roll uses React/AppSetup/RCTAppSetupUtils.mm to create its modules?
@cipolleschi, I think we should definitely re-think this utility:
react-native/React/AppSetup/RCTAppSetupUtils.mm
Lines 72 to 97 in 222dd96
id<RCTTurboModule> RCTAppSetupDefaultModuleFromClass(Class moduleClass) | |
{ | |
// Set up the default RCTImageLoader and RCTNetworking modules. | |
if (moduleClass == RCTImageLoader.class) { | |
return [[moduleClass alloc] initWithRedirectDelegate:nil | |
loadersProvider:^NSArray<id<RCTImageURLLoader>> *(RCTModuleRegistry *moduleRegistry) { | |
return @[ [RCTLocalAssetImageLoader new] ]; | |
} | |
decodersProvider:^NSArray<id<RCTImageDataDecoder>> *(RCTModuleRegistry *moduleRegistry) { | |
return @[ [RCTGIFImageDecoder new] ]; | |
}]; | |
} else if (moduleClass == RCTNetworking.class) { | |
return [[moduleClass alloc] | |
initWithHandlersProvider:^NSArray<id<RCTURLRequestHandler>> *(RCTModuleRegistry *moduleRegistry) { | |
NSArray *handlers = [moduleRegistry modulesConformingToProtocol:@protocol(RCTURLRequestHandler)]; | |
NSArray *handlersWithDefaultOnes = [handlers arrayByAddingObjectsFromArray: @[ | |
[RCTHTTPRequestHandler new], | |
[RCTDataRequestHandler new], | |
[RCTFileRequestHandler new], | |
]]; | |
return handlersWithDefaultOnes; | |
}]; | |
} | |
// No custom initializer here. | |
return [moduleClass new]; | |
} |
It locks people into pre-customized networking and image loader modules.
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.
@RSNara yes, I think that we should step back and have a look at how we register components and turbomodules in the users apps as we cannot ask them to manually register those elements. Right now we are working around it using the CLI.
(as a side note: those methods where there before I joined. I don't have context as why they are the way they are. 😅 )
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.
@WoLewicki, could you describe your problem in more details? Is the problem that React Native Camera Roll uses React/AppSetup/RCTAppSetupUtils.mm to create its modules?
@RSNara I am only porting the cameraroll
to new arch so I don't have too deep knowledge, but it seems that the module just wants to handle iOS
photos in requests and it seems to have been done by implementing RCTURLRequestHandler
protocol on Paper
in order for the module to be taken into account when parsing requests in RCTNetworking.mm
. It does not work on new arch when module being converted to turboModule though.
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
cc @cipolleschi @RSNara did you come to a conclusion of how should this be handled? |
Same as per the other PR, I created an internal tasks and we are going to discuss it on Monday or Wednesday. |
Hi @WoLewicki, sorry for the long wait. To sumup the discussion:
One way to achieve this that I was thinking about follow roughly these lines:
@RSNara @WoLewicki what do you think about the approach? |
LGTM, it adds some complexity, but there are probably very little libraries that would like to do it so I'm ok with it 🚀 |
On the library side, it would be just to specify an extra line in the |
Oh ok, then it seems like a perfect option. |
Closing this as commit 4236538 landed and it implements a mechanism to link custom modules that conforms to some specific React Native protocols. |
Summary
Right now modules conforming to
RCTURLRequestHandler
protocol are hardcoded on new arch and there is no way to add your own modules there. I added the first look on a method providing all such modules there. The work is connected with bringing new arch toreact-native-cameraroll
: react-native-cameraroll/react-native-cameraroll#460. For now I didn't convert theRNCAssetsLibraryRequestHandler
from that lib toTurboModule
since there is no way to get it then.Changelog
[IOS] [ADDED] - custom
RCTURLRequestHandler
modules on new archTest Plan
Try and register a custom module conforming to
RCTURLRequestHandler
on new arch.