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

fix: add custom request handlers on new arch #36071

Closed
wants to merge 1 commit into from
Closed

fix: add custom request handlers on new arch #36071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions React/AppSetup/RCTAppSetupUtils.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,13 @@ void RCTAppSetupPrepareApp(UIApplication *application, BOOL turboModuleEnabled)
} else if (moduleClass == RCTNetworking.class) {
return [[moduleClass alloc]
initWithHandlersProvider:^NSArray<id<RCTURLRequestHandler>> *(RCTModuleRegistry *moduleRegistry) {
return @[
[RCTHTTPRequestHandler new],
[RCTDataRequestHandler new],
[RCTFileRequestHandler new],
];
NSArray *handlers = [moduleRegistry modulesConformingToProtocol:@protocol(RCTURLRequestHandler)];
NSArray *handlersWithDefaultOnes = [handlers arrayByAddingObjectsFromArray: @[
[RCTHTTPRequestHandler new],
[RCTDataRequestHandler new],
[RCTFileRequestHandler new],
]];
return handlersWithDefaultOnes;
}];
}
// No custom initializer here.
Expand Down
1 change: 1 addition & 0 deletions React/Base/RCTBridgeModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ RCT_EXTERN_C_END

- (id)moduleForName:(const char *)moduleName;
- (id)moduleForName:(const char *)moduleName lazilyLoadIfNecessary:(BOOL)lazilyLoad;
- (NSArray *)modulesConformingToProtocol:(Protocol *)protocol;
@end

typedef UIView * (^RCTBridgelessComponentViewProvider)(NSNumber *);
Expand Down
12 changes: 12 additions & 0 deletions React/Base/RCTModuleRegistry.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,16 @@ - (id)moduleForName:(const char *)moduleName lazilyLoadIfNecessary:(BOOL)lazilyL
return module;
}

- (NSArray *)modulesConformingToProtocol:(Protocol *)protocol
{
NSMutableArray *modules = [NSMutableArray new];
RCTBridge *bridge = _bridge;
if (bridge) {
[modules addObjectsFromArray:[bridge modulesConformingToProtocol:protocol]];
}

// TODO: find a way to get the same information of turboModules
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@RSNara RSNara Feb 14, 2023 •
edited
Loading

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.

Copy link
Contributor

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. 😅 )

Copy link
Contributor Author

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.

return [modules copy];
}

@end