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.

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

Add FabricExample application with the new arch enabled #911

Conversation

j-piasecki
Copy link
Contributor

Summary

This PR adds a new example app using RN 0.71 with the new architecture enabled by default. This was used to test #910.

It's not necessary for this PR to be merged in order to use the one converting the library to turbomodule as the app may be removed. Setting the base to this one should make relevant changes easier to check.

@tido64
Copy link
Member

tido64 commented Feb 7, 2023

Hi, thanks for working on this. I see you're trying to add TurboModule support. Have you had a look at how the other example app works? We are using react-native-test-app, which allows us to easily enable TurboModule (for both Android and iOS/macOS). Would it make more sense to reuse the existing test app and instead work on enabling building of both Legacy and New Architecture on CI?

cc @krizzu

@j-piasecki
Copy link
Contributor Author

j-piasecki commented Feb 7, 2023

I don't have experience with react-native-test-app, what's the correct way to upgrade RN version? I didn't see anything about it on the GitHub wiki.

A potential problem could be that the library would work only on the RN 0.70 and higher when using the new arch due to a different codegen config, and It seems like neither 0.70, nor 0.71 is not fully supported yet.

I could close this PR and remove the commits related to the new app from the other PR so you could set it up correctly.

@tido64
Copy link
Member

tido64 commented Feb 7, 2023

I don't have experience with react-native-test-app, what's the correct way to upgrade RN version? I didn't see anything about it on the GitHub wiki.

If you read the motivation, the whole purpose of the project is to make upgrades transparent. You need only bump the version number in package.json.

A potential problem could be that the library would work only on the RN 0.70 and higher when using the new arch due to a different codegen config, and It seems like neither 0.70, nor 0.71 is not fully supported yet.

The table is incomplete because react-native-macos is going to jump from 0.68 to 0.71, essentially skipping 0.69 and 0.70. Otherwise, react-native-test-app supports 0.71.

I could close this PR and remove the commits related to the new app from the other PR so you could set it up correctly.

Please give it a try. You should be able to just bump react-native and react-native-windows to 0.71 and follow these instructions to enable New Arch for Android and iOS. You might want to temporarily remove react-native-macos to avoid problems. If you hit any issues, let me know.

Edit: Just noticed that we're on a slightly older version of react-native-test-app. I can bump it now.

@j-piasecki
Copy link
Contributor Author

I've tried to implement it, but after upgrading to React Native 0.71, the pod install started failing with:

[!] Invalid `Podfile` file: 783: unexpected token at 'Config Validation Error: "project.ios.project" is not allowed

I've changed the project field to sourceDir and got the following:

Invalid `Podfile` file: undefined method `[]' for nil:NilClass.

 #  from /Users/jakubpiasecki/Projects/react-native-async-storage/example/ios/Podfile:6
 #  -------------------------------------------
 #  use_flipper! false
 >  use_test_app! do |target|
 #    target.app do
 #  -------------------------------------------

On Android I had to add @OptIn(ExperimentalStdlibApi::class) annotation to this method due to buildList being marked with ExperimentalStdlibApi.

After that I got:

Execution failed for task ':react-native-async-storage_async-storage:buildCodegenCLI'.
> A problem occurred starting process 'command '/Users/jakubpiasecki/Projects/react-native-async-storage/example/node_modules/react-native-codegen/scripts/oss/build.sh''

which I believe is due to facebook/react-native#35495.

@tido64
Copy link
Member

tido64 commented Feb 8, 2023

I've tried to implement it, but after upgrading to React Native 0.71, the pod install started failing with…

#915 should fix it.

On Android I had to add @OptIn(ExperimentalStdlibApi::class) annotation to this method due to buildList being marked with ExperimentalStdlibApi.

This one I'm unfamiliar with. @krizzu do you know why we have to do this?

@krizzu
Copy link
Member

krizzu commented Feb 8, 2023

On Android I had to add @OptIn(ExperimentalStdlibApi::class) annotation to this method due to buildList being marked with ExperimentalStdlibApi.

This one I'm unfamiliar with. @krizzu do you know why we have to do this?

What version of Kotlin is used? In 1.6 buildList became stable

@j-piasecki
Copy link
Contributor Author

@krizzu You're right, it seems like my Android Studio decided to use Kotlin 1.5. I've just tried building from CLI and I didn't get this error.

@j-piasecki
Copy link
Contributor Author

#915 should fix it.

It fixes the first problem, however the second one:

Invalid `Podfile` file: undefined method `[]' for nil:NilClass.

 #  from /Users/jakubpiasecki/Projects/react-native-async-storage/example/ios/Podfile:6
 #  -------------------------------------------
 #  use_flipper! false
 >  use_test_app! do |target|
 #    target.app do
 #  -------------------------------------------

is still there.

@tido64
Copy link
Member

tido64 commented Feb 8, 2023

is still there.

Sorry, I missed that CLI requires sourceDir. I've updated the PR and pod install should work now. Please make sure you run pod install from example/ios and not use --project-directory=example/ios. I've just discovered that a bug that I thought I had fixed in react-native related to this.

Update: Upstream PR: facebook/react-native#36096

@j-piasecki
Copy link
Contributor Author

Ok, I've got it to work on both Android and iOS. For now I've added a quick fix for the codegen issue linked above: abed02f#diff-ed8534ece8080a880075e103269eb1b9bdff22e8dfcf8427d7d977603292b889R59-R65. I guess this PR can now be closed as #910 no longer requires it?

Thanks for your help!

@tido64
Copy link
Member

tido64 commented Feb 9, 2023

Ok, I've got it to work on both Android and iOS.

Thank you for working on this. Please let me know if you hit other issues with the test app. I'll try to wrap up #915 soon.

@tido64 tido64 closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants