-
Notifications
You must be signed in to change notification settings - Fork 1.9k
android(feat):new arch support #4675
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
Conversation
# Conflicts: # detox/android/detox/src/full/java/com/wix/detox/reactnative/idlingresources/animations/AnimatedModuleIdlingResource.kt
Change the way of getting is fabric enabled
Change the way of getting is fabric enabled
@gosha212 I think the test app will crash on the Localization screen on Android, since it uses a legacy native module. |
we should check if our bridge native modules work with backward comp. or if there's something magically /hybrid that happens here |
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.
In iOS we can check the env flag process.env.RCT_NEW_ARCH_ENABLED
We can use this when running new arch on android as well (for the tests)
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.
Overall, great job. Lot's of notes on my end but we are nevertheless very close to finish 👑
.../demo-react-native/android/app/src/androidTest/java/com/example/DetoxTestAppJUnitRunner.java
Show resolved
Hide resolved
examples/demo-react-native/android/app/src/main/java/com/example/MainActivity.kt
Show resolved
Hide resolved
detox/android/detox/src/full/java/com/wix/detox/reactnative/reloader/ReactNativeReloader.kt
Outdated
Show resolved
Hide resolved
...src/full/java/com/wix/detox/reactnative/idlingresources/timers/FabricTimersIdlingResource.kt
Outdated
Show resolved
Hide resolved
...src/full/java/com/wix/detox/reactnative/idlingresources/timers/FabricTimersIdlingResource.kt
Show resolved
Hide resolved
...src/full/java/com/wix/detox/reactnative/idlingresources/timers/FabricTimersIdlingResource.kt
Outdated
Show resolved
Hide resolved
...src/full/java/com/wix/detox/reactnative/idlingresources/timers/FabricTimersIdlingResource.kt
Outdated
Show resolved
Hide resolved
…lingresources/uimodule/fabric/FabricUIManagerIdlingResources.kt Co-authored-by: d4vidi <[email protected]>
Co-authored-by: d4vidi <[email protected]>
detox/android/detox/build.gradle
Outdated
api('androidx.test:core-ktx:1.6.1') { | ||
because 'Needed by Detox but also makes AndroidX test core seamlessly provided to Detox users with hybrid apps/E2E-tests.' | ||
} | ||
api("org.jetbrains.kotlin:kotlin-reflect:$_kotlinVersion") { | ||
because('Needed by Detox for kotlin reflection') | ||
} |
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.
on 2nd thought, maybe implementation
would be a better idea here
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.
...om/wix/detox/reactnative/idlingresources/factory/FabricDetoxIdlingResourceFactoryStrategy.kt
Outdated
Show resolved
Hide resolved
.../detox/src/full/java/com/wix/detox/reactnative/idlingresources/timers/JavaTimersReflected.kt
Show resolved
Hide resolved
Hey @gosha212 - just wanted to say thanks for the new arch fixes in detox - I know it's a pain getting the ReactContext because I also had to fight my way through it in react-native-firebase and notifee, with all the symbols being different across versions but you need it to work on all the versions too, what a pain. I tried the 20.33.0-prerelease.0 release and it works for me (at least with one trivial build.gradle patch that's already tracked here in an issue). 20.32.0 definitely didn't get the react context in new arch mode, but I've got all the correct adb logcat messages now indicating it has it with these PRs in the prerelease 🔥 |
@mikehardy thanks for the warm words. The iOS is 99% ready |
react-native-firebase e2e app (my use case here) does almost no app interaction so we are not a great indicator of full-feature functionality, but it's working for us on react-native iOS platform as well as react-native-macos already, for basic app launch / terminate / ready-signaling behavior, so I'm well-pleased. Merged the PR with it already and our e2e app is able to test (and sadly, uncover some) new architecture issues |
@mikehardy thanks again for the kind words! 🙏🏻
Music to our ears... 🎶 whenever tests uncover issues, we feel better about our work 😊 |
Description
In this pull request, I have added support for the new arch in detox