-
Notifications
You must be signed in to change notification settings - Fork 162
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
Adds a bubblewrap install
command
#161
Adds a bubblewrap install
command
#161
Conversation
`bubblewrap install` allows developers to install an APK to a connected development device to test the application without the need to find the correct adb commands.
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.
Code all looks good - one or two UI comments.
packages/cli/src/lib/cmds/install.ts
Outdated
// parameter 0 would be the path to 'node', followed by `bubblewrap.js` at 1, then `install` at | ||
// 2. So, we want to start collecting args from parameter 3 and ignore any a possible | ||
// `--apkFile`, which is specific to install. Extra parameters are passed through to `adb`. | ||
const originalArgs = process.argv.slice(3).filter((v) => !v.startsWith(APK_FILE_PARAM)); |
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 think this is clever, but I'm not 100% sure what we're trying to do. We're making a bubblewrap install
behave almost exactly like adb install
, but letting the user enter --apkFile
anywhere (although that does seem to be optional). It seems that we're trying to be very helpful but I'm wondering if we could end up being confusing.
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.
Someone using Bubblewrap may not have adb
in ther PATH
. The goal is just to make it easier to call adb install
. It defaults to using the default output file name, so bubblewrap install
should be enough in most cases. But if, for instance, you have more than one device connected, you may want to pass extra parameters to ADB, and this allows that.
'', | ||
'', | ||
'Options: ', | ||
'--apkFile ................. path to the APK file to be isntalled. Defaults to ' + |
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.
What do you think about adding some option (maybe like debug
or verbose
) that prints out the adb command we're executing?
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.
done
]; | ||
|
||
tests.forEach((test) => { | ||
it(`Build the correct install commandon ${test.platform}`, async () => { |
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.
nit: typo
--verbose flag prints the full adb command being executed.
bubblewrap install
allows developers to install an APK to aconnected development device to test the application without
the need to find the correct adb commands.
Closes #49