-
Notifications
You must be signed in to change notification settings - Fork 932
fix: use variant arg in build-android command #1798
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
fix: use variant arg in build-android command #1798
Conversation
|
Hey @adamTrz, after some conversation on issue #1796, I found that docs are not updated, see here. It can be confusing - #1796 (comment), and actually we've that in the codebase, here, so you could you please add info about that |
c659e09 to
d2fa723
Compare
packages/cli-platform-android/src/commands/runAndroid/runOnAllDevices.ts
Outdated
Show resolved
Hide resolved
d2fa723 to
d3ed617
Compare
| import tryLaunchEmulator from './tryLaunchEmulator'; | ||
| import tryInstallAppOnDevice from './tryInstallAppOnDevice'; | ||
| import {Flags} from '.'; | ||
| import {BuildFlags} from '../buildAndroid'; |
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.
now we have a circular reference between buildAndroid and runOnAllDevices
| export function getTaskNames( | ||
| appName: string, | ||
| commands: Array<string>, | ||
| args: BuildFlags, |
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.
I'm not a fan of leaking whole args to a helper function. Can we convert that into explicit arguments? Either as an object or a arg list, I'm fine with both
thymikee
left a comment
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.
Let's fix that circular reference and getTaskNames fn signature and we can ship it
d3ed617 to
0705f50
Compare
packages/cli-platform-android/src/commands/runAndroid/runOnAllDevices.ts
Show resolved
Hide resolved
|
@adamTrz please verify my latest updates and we're good to go :) |
Summary:
Fixes: facebook/react-native#35838
If
variantis passed torun-androidcommand we still should respect it.Test Plan:
yarn react-native run-android --mode FullDebug --verboseshould output./gradlew app:installFullDebugyarn react-native run-android --variant FullDebug --verboseshould output./gradlew app:installFullDebugyarn react-native build-android --mode FullDebug --verboseshould output./gradlew app:assembleFullDebugyarn react-native build-android --variant FullDebug --verboseshould output./gradlew app:assembleFullDebug