feat: add new arch #119
feat: add new arch #119
Conversation
|
Thanks for the PR, would you care to explain what are the benefits on converting this to a turbo module? I can see some of the code aims to bring compatibility back with < 71 versions, but it is not clear to me if it is connected to all the turbo module changes |
| @@ -26,6 +26,10 @@ def isNewArchitectureEnabled() { | |||
| return project.hasProperty("newArchEnabled") && project.newArchEnabled == "true" | |||
| } | |||
|
|
|||
| if (isNewArchitectureEnabled()) { | |||
| apply plugin: "com.facebook.react" | |||
There was a problem hiding this comment.
when I was adding support for 0.71 this caused errors regarding duplicate symbols and caused conflicts with other libraries, is this correct?
There was a problem hiding this comment.
I didn't see anything like this to be honest, and if I removed it, it seems like the codegen does not generate specs, so it is necessary 😅 You can see this step here too: https://github.com/react-native-community/RNNewArchitectureLibraries/tree/feat/back-turbomodule-070#turbomodule-set-up-buildgradle
| } | ||
|
|
||
| @Override | ||
| public ReactModuleInfoProvider getReactModuleInfoProvider() { |
There was a problem hiding this comment.
This looks complex, I guess it is necessary to register the turbomodule, can you explain a bit what it does? It looks like some kind of description
There was a problem hiding this comment.
If you look deeper into that method, it is a kind of a description of the module. I didn't debug where it is used exactly though.
|
As for the benefits of the conversion, the |
|
The install method was already synchronous though? And yes, indeed most of the time the time apps will initialize the module on app start... While I do appreciate the effort in opening the PR it does introduce complexity and there is code there that doesn't seem to be documented anywhere, the old bridge implementation was straightforward, while the TurboModules require this complex setup |
It was synchronous but caused all the other queues to be blocked for during this method's dispatching iirc which is not the case with JSI I believe.
I understand your point, but I wouldn't call this setup complex. It is still compatible with the old arch and makes the library subscribe to the newest changes and the future features of the framework. |
But... any sync JS call will block anyways? am I misunderstanding something?
But the library is already compatible with the old arch and with the new one, at least during my testing. Am I also wrong here? I also really dislike turbo modules, this code generation step is, in my opinion, the wrong path, by skipping this I have complete control over how the module is initialized. I have to disagree to the point of it not being complex, that capabilities descriptor class on java looks über complicated and it is only referenced via source code to some example? |
|
I also don't know what's the cost of initializing this module... I could guess it is very low since the bindings are not that large... but without numbers it is hard to tell if this is premature optimization |
It will block only the queue on which it will be dispatched imo.
You are right.
Yeah for sure. The main reason for this PR is subscribing to the newest solutions of the framework. I suspect there will be more things added to the |
|
really thorn on this... I will leave the PR open for now, the only real benefit IMO is somehow future-proofing the package and if I merge I will have to maintain it in the future which means keeping up with the kerfuffle of TurboModules for which I have no free time. |
|
There is no hurry with it I believe. If you are not sure about the current benefits, I think it is OK just to leave this open and let people just use the version from this branch if they want it now. |
PR converting module to
TurboModule.