Major Pain Points to Migrate to the New Architecture #68
Replies: 14 comments 84 replies
-
|
Pain : Hard to see what should be updated for libraries for every release. Idea 💡 ? Create |
Beta Was this translation helpful? Give feedback.
-
|
I'm probably misremembering but don't libraries now need to have a fixed folly version in their source code (for new arch stuff)? that would hinder the ability to support multiple versions of RN - but again, I might be misremembering |
Beta Was this translation helpful? Give feedback.
-
|
It's not an issue anymore. AFAIK it's removed from the docs and multiple libs (RNGH, safearea, slider) already |
Beta Was this translation helpful? Give feedback.
-
Yeah for some libraries (for example) WebView, I've nearly never had to do things to support news releases, but since the new arch seems to have more config files for libraries I am worried ! |
Beta Was this translation helpful? Give feedback.
-
|
yh i'm noticing too atm 0.70.0-rc.2 + new arch + RNGH or [!] CocoaPods could not find compatible versions for pod "RCT-Folly":
In snapshot (Podfile.lock):
RCT-Folly (from `../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec`)
In Podfile:
RCT-Folly (from `../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec`)
RNScreens (from `../node_modules/react-native-screens`) was resolved to 3.15.0, which depends on
RCT-Folly (= 2021.06.28.00-v2)
React-RCTVibration (from `../node_modules/react-native/Libraries/Vibration`) was resolved to 0.70.0-rc.2, which depends on
RCT-Folly (= 2021.07.22.00)from the folly bump couple mo ago
so some might need a patch till next release |
Beta Was this translation helpful? Give feedback.
-
|
Unfortunately yes. It was a bad suggestion from our side, asking the library owners to add the The patch should be really quick: just drop the Dropping the |
Beta Was this translation helpful? Give feedback.
-
|
Pain : Since library users aren't always on latest, it's hard to know when to start migrating to new API, or the minimum version for each API in order to announce with breaking release the versions dropped Example : Let's use |
Beta Was this translation helpful? Give feedback.
-
|
Pain: hard to know which libraries support the new architecture programmatically. How to solve: maybe we could somehow enforce a field in the package.json that declares support? That way that info could be parsed by things like reactnative.directory and dep-check |
Beta Was this translation helpful? Give feedback.
-
how can an app migrate to the new arch if they can't tell if the libs they use are ready? If the flow is just try -> something breaks -> spend time finding out the error -> open an issue on that lib -> wait for it to be fixed -> try again then this is not sustainable. |
Beta Was this translation helpful? Give feedback.
-
|
or are you suggesting that everyone should migrate to the new arch after new arch is the default? |
Beta Was this translation helpful? Give feedback.
-
Sadly the version field won't wrok IMHO as libraries can just update the React Native version and opt-out of the New Architecture (in a sense it's a same with React 18). We need a way to programmatically know if a library is supporting it or not without having to check the source code. As @cipolleschi mentioned, |
Beta Was this translation helpful? Give feedback.
-
How we did it for React 18: First we shipped new APIs and renderers with an Once we felt confident that we were ready to ship the new renderer in the very next version of React, we removed the unstable prefixes and published the first React 18 alpha. This version added a warning when using the legacy renderer, forcing you to upgrade to the new renderer. This warning means that if you're on react@18, you've migrated. So "supporting react@18" means supporting the new renderer. Throughout the alpha, beta, phases we continued to recommend not using it in production, which gave time for libraries to add support targeting the alpha and beta tags. Once the release was ready to publish, we did a few rounds of RCs to notify users that stable was coming and to ask power users to try it in production. Finally, we published React 18 stable, cutting over to the new renderer and recommending usage in production. This means that libraries are able to target react@18 to indicate that they support the new renderer. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @rickhanlonii for sharing context. However, we can't forget that things move much more slower on mobile than on web. I can see us maintaining the Old Architecture for 1-2 years after the new one is enabled by default, mainly because many people works on OSS library in their free time and the migration is not painless and immediate. I feel what @kelset says: trial-and-error in this context is quite painful and extremely time consuming. We should find a better way. |
Beta Was this translation helpful? Give feedback.
-
|
Pain: there's no public/official benchmarks to prove that the new architecture leads to improvements. It might sound silly but without this type of "marketing" it's harder to sell to teams and their managers why they should undergo the transition to the new architecture. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed. And it's linked to companies sponsoring the time to spend on OSS libraries to support new architecture. It's not always devs deciding on that, but we need all the libraries. |
Beta Was this translation helpful? Give feedback.
-
|
Here you can find some benchmarks https://twitter.com/Tr0zZe/status/1572897540122574849 . There are also plans to compare it with the old arch |
Beta Was this translation helpful? Give feedback.
-
|
When a library uses c++ state there are lots of manual build changes required in userland on Android. The library is opting out of part of the codegen and providing custom This is a common scenario because it affects any library that calls |
Beta Was this translation helpful? Give feedback.
-
|
As far as I understood, CPP files in Android should go in a folder like One test you could do, is to create a new react native app with And then add you library as a dependency (`yarn add <path_to_navigation>/navigation) and try to build. ). RN 0.70.0-rc.2 already supports autolinking! |
Beta Was this translation helpful? Give feedback.
-
|
Please can you explain more about how autolinking works for libraries using c++ state. These libraries have to hand-write custom |
Beta Was this translation helpful? Give feedback.
-
Here the gist of it: https://github.com/react-native-community/cli/blob/main/docs/autolinking.md#platform-android Starting from 0.70, we've adapted the New template to include teh Component Registry from the RN CLI: The |
Beta Was this translation helpful? Give feedback.
-
|
@cortinico @thymikee I've tried autolinking in React Native 0.70.0 but it doesn't work for components that use c++ state. These components have Please can you tell me how I can get my hand-written ComponentDescriptors included in the generated |
Beta Was this translation helpful? Give feedback.
-
|
Thanks has been answered thanks to react native screens and @kkafar |
Beta Was this translation helpful? Give feedback.
-
|
Pain: There is no official guide, how to get view measurements before commit phase on the native side. Description: In my library, I would like to update native parent view (HostView), when react native child dimensions changed. I would like to do it synchronous manner instead of using How to solve it:
|
Beta Was this translation helpful? Give feedback.
-
|
Same here. |
Beta Was this translation helpful? Give feedback.
-
|
I think better documentation with typescript codegen can be helpful. I faced an issue for the below use case that mixed types are not supported with codegen yet. RN's ImageSource is a mixed type but it works with codegen. So I am not sure whether I am doing something wrong. Below doesn't worktype Source = { uri: string }
export interface NativeProps extends ViewProps {
source?: number | Source
}This worksimport type { ImageSource } from 'react-native/Libraries/Image/ImageSource'
export interface NativeProps extends ViewProps {
source?: ImageSource
}P.S. The mixed type error also happens when using flow |
Beta Was this translation helpful? Give feedback.
-
|
I believe this was already reported by @Titozzz here right? #27 (comment) |
Beta Was this translation helpful? Give feedback.
-
|
sorry, I missed checking that discussion.
Kind of yes, although this one is union type and that comment is for intersection types. Just wanted to bring that TS documentation felt like a pain point for me. A doc for unsupported types/cases could be useful. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @intergalacticspacehighway, we recently updated the documentation and added the TypeScript section in the appendix. As you correctly guessed, enum types are not currently supported by the the Codegen, not even in Flow. There is only a special case when they are supported and it is in Callbacks. About I agree that it is not ideal: we hope to find enough resources to improve the Codegen in the next future. |
Beta Was this translation helpful? Give feedback.
-
|
In addition to updating docs, what if the codegen output better errors when using unknown/unsupported types? |
Beta Was this translation helpful? Give feedback.
-
|
Another note: codegen special casing |
Beta Was this translation helpful? Give feedback.
This comment has been hidden.
This comment has been hidden.
-
|
Oh, yes! Autolinking in Android was not supported before 0.70! |
Beta Was this translation helpful? Give feedback.
-
|
Just to make sure we're talking about the same thing, this is what I'm talking about: https://github.com/oblador/react-native-performance/blob/03b15a9fd6c3d0ff42087586d56e269baacb8b1d/packages/react-native-performance/android/build.gradle#L111-L117 |
Beta Was this translation helpful? Give feedback.
-
|
After talking at the Summit, and verifying that it work, we can consider this no longer a pain point, right? |
Beta Was this translation helpful? Give feedback.
-
|
Yep! 👍 |
Beta Was this translation helpful? Give feedback.
-
|
I'm hiding this as 'Resolved'. Thanks for your input @oblador |
Beta Was this translation helpful? Give feedback.
-
iOS folly setupIt will become a burden on maintainers to keep the folly/cpp version and flags in sync with core. Seems that it will be tricky to support multiple react-native versions simultaneously. As a lib maintainer these seems more like app/core dependencies that I don't want to have to care about, and rather just depend on a versionless |
Beta Was this translation helpful? Give feedback.
-
|
You don't need to specify the software-mansion/react-native-screens#1549 |
Beta Was this translation helpful? Give feedback.
-
|
As @satya164 was saying, that's not an issue anymore. Asking to specify the |
Beta Was this translation helpful? Give feedback.
-
Code gen and backwards supportUsing code-gen and keeping old-arch support is annoying – can't use complex types like typed objects and haven't found a way to extend the ManagerSpec on Android. Most non-trivial projects I've seen simply commit the generated code and core cheat by always generating code regardless of arch. |
Beta Was this translation helpful? Give feedback.
-
|
@oblador can you expand on this? What do you mean by "simply commit the generated code and core cheat by always generating code regardless of arch." |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
Example of commited codegen: https://github.com/software-mansion/react-native-screens/blob/main/android/src/paper/java/com/facebook/react/viewmanagers/RNSScreenStackHeaderConfigManagerInterface.java |
Beta Was this translation helpful? Give feedback.
-
|
And I think this is the code that does codegen for react-native libs in old arch: https://github.com/facebook/react-native/blob/7cc2d1a24983d00282fcc694986faad88991845e/scripts/cocoapods/codegen.rb#L95-L101 |
Beta Was this translation helpful? Give feedback.
-
|
My 2 cents here: this is a symptom of having the codegen invocation happening at the app level. Currently a library author needs to offer 2 sourcesets (oldarch/newarch) because the app will be responsible of invoking the codegen for all the libraries. This means that if you're relying on codegen, and the app doens't invoke it (because is on a older version of RN) you will be missing sources and your library will fail to compile. IMHO this should be solved by moving the responsibility of invoking the codegen to the library space. Using the codegen should be an "implementation detail" of the library. |
Beta Was this translation helpful? Give feedback.
-
|
Noticed that optional type doesn't work well with codegen. Is it supported? e.g. Consider this turbomodule spec If a call is made
|
Beta Was this translation helpful? Give feedback.
-
|
Hey @cipolleschi! Did anything come out of this? I'm using TypeScript with a method signature that expects an optional value using the |
Beta Was this translation helpful? Give feedback.
-
|
Hi there! I manage to dig a little into this issue. So, the problem is more convoluted that what it looks like. A function like func foo(parameter: string, optionalParameter?: string) {}can be invoked in two ways: foo("bar");
// or
foo("bar", "baz");when this happens on a NativeModule, React Native transform this function call in something that can be passed to the C++ layer. If you use the first form, it will try to invoke something like (this is an approximation, with namespaces to be clearer): react::facebook::YourNativeModule::foo(std::string parameter);when using the second form, it will do something like: react::facebook::YourNativeModule::foo(std::string parameter, std::string optionalParameter);The problem is that Codegen does not generate the two C++ methods, but it only generates the full form (with two parameters). That's why, when calling Codegen cosiders optional parameters as nullable parameters, and it creates a signature where you can pass I agree that the semantic is different and this is not ideal. We can fix that in the future, probably. I'm currently working internally to understand when and how we can prioritize this (and other similar) works. |
Beta Was this translation helpful? Give feedback.
-
|
@cipolleschi Thanks for the insight! It looks like I'm observing the same thing when inspecting the generated Objective-C headers. Ideally, a fix would be a good way forward, but do you know if there are any ways to work around this other than creating separate method APIs? A lot of our methods have optional callback parameters, for example. |
Beta Was this translation helpful? Give feedback.
-
I understand. We are trying to figure out how to improve codegen in a more holistic way. I'll definitely keep this feedback in mind for the next planning session! Thank you so much!
What I mean is that you can create your Spec (sorry for the syntax, it is a simplified TS, but I think it conveys the idea): type Spec extends Turbomodule {
foo(parameter: string, optParameter?: string): void
}And then wrap it into a proper module that does some conversions for you: type ModuleAPI {
foor(parameter: string, optParameter?: string): void {
const specs = new Spec();
const optionalParameter = optParameter ? optParameter : null;
specs.foo(parameter, optionalParameter);
}
}Or something similar, and then the rest of your code only uses I know it is not ideal and it introduces a bit of replication, but it also allow you to decouple your APIs from the specs which is, in general, a good thing. What do you think? This should unblock you for the time being. I'll try to keep you updated on further developments! |
Beta Was this translation helpful? Give feedback.
-
|
Hey @cipolleschi, using a wrapper layer sounds like a good workaround and seems to be working so far! I'll reach back out if I have any further questions. |
Beta Was this translation helpful? Give feedback.
This comment was marked as off-topic.
This comment was marked as off-topic.
-
|
Hi @intergalacticspacehighway, thanks for the question, but I think that this is not the right thread for this. Here we want to collect issues people are facing on migrating to the New Architecture, not just bugs or simple questions about it. Could you perhaps move this question in a thread in the Q&A section? |
Beta Was this translation helpful? Give feedback.
-
|
Since @cortinico mentioned it and I had completely forgotten it: Build times ⏱In Another concern with such build times is that people can easily burn through their monthly allotted Actions minutes, which we only get 2000 of on the free tier:
With the build times being what it is, I can't set up CI to build/test all the new arch stuff. It would be great if we could get pre-built binaries in some form for iOS. |
Beta Was this translation helpful? Give feedback.
-
|
@kelset good catch, it was 0.12.0, but i can confirm there's |
Beta Was this translation helpful? Give feedback.
-
because the cwd is in the folder with cloned hermes, the hermes-engine.podspec here is the one from hermes repo. update my previous proposal diff to address this issue. basically passing the information from spec to environment variables. |
Beta Was this translation helpful? Give feedback.
-
|
thanks for your work @Kudo! I was able to verify that now it works correctly (see here) so I went ahead and setup a PR for it: facebook/react-native#34710 |
Beta Was this translation helpful? Give feedback.
-
|
I just tried upgrading my app to 0.70.6 to see if it even worked — aside from the fact that I could not solve the hundreds of build errors even though I followed the upgrade helper directions correctly, building always took more than 20 minutes EACH time, being stuck for most of it in this phase: :ReactAndroid:buildCMakeDebug[x86][fabricjni,hermes-exec... |
Beta Was this translation helpful? Give feedback.
-
Thanks for your feedback @DominickVale |
Beta Was this translation helpful? Give feedback.
-
|
Pain: Is there any other alternative? |
Beta Was this translation helpful? Give feedback.
-
|
Hi @rushatgabhane, thank you for the question. I don't have enough context to help you out directly with this question, but perhaps @sammy-SC can help. What do you think? |
Beta Was this translation helpful? Give feedback.
-
|
Also, @rushatgabhane could you prepare a repro where we can see the performance impact of moving |
Beta Was this translation helpful? Give feedback.
-
|
Hello @rushatgabhane, may I ask where are you encountering performance issues with |
Beta Was this translation helpful? Give feedback.
-
|
The following is taken from the Direct Manipulation page of the React Native docs. @sammy-SC @cipolleschi Are you saying the new React Native architecture has made these considerations outdated?
|
Beta Was this translation helpful? Give feedback.
-
|
|
Beta Was this translation helpful? Give feedback.
-
Hi everyone! We want to start a discussion to collect feedback from Core Contributors and Partners about major pain points on migrating to the new architecture.
These pain points can be of any nature:
The goal is to highlight what are the most relevant issues so that we can work on them together.
To Add a Pain Point
Simply post a message in the discussion, describing the problem and why it makes it hard to migrate to the new architecture. Try to be as specific as possible.
Each pain point will be then added to the Poll Options so other people can vote it and we also have a recap of all the pain points.Poll does not offer multiple choices, so you could only vouch for one issue, which is not optimal.
I replaced the poll with the list below, which links to the reported issue.
Please, upvote the issues you care the most! This will give us a signal of how impactful the issue is.
setNativePropsis being deprecated, and the docs say that we should move all to state. This has a performance impact4 votes ·
Beta Was this translation helpful? Give feedback.
All reactions