-
Notifications
You must be signed in to change notification settings - Fork 6
Cordova support, Code-push App Name , Update dependencies #5
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
base: master
Are you sure you want to change the base?
Conversation
This is due to the fact that the isparta library is no longer maintained.
Updated description e keywords packages
Codecov Report
@@ Coverage Diff @@
## master #5 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 4
Lines ? 24
Branches ? 0
=======================================
Hits ? 24
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
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.
Hi, thanks for the PR, it looks interesting!
Here some small fix before merging
Can you update the README with the new cli commands? Thanks
@@ -1,5 +1,5 @@ | |||
import codepushLogin from "./steps/login"; | |||
import codepushReleaseReact from "./steps/release-react"; | |||
import codePushRelease from "./steps/release-react"; |
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.
The file name should be modified, maybe in release.js, because it is also for cordova release
platform, | ||
`-d "${argv.deploymentName}"`, | ||
`--des "${argv.description}"`, | ||
`--dev ${argv.development}`, | ||
(!argv.framework || argv.framework === "reactnative") ? `--dev ${argv.development}` : "", | ||
`-m ${argv.mandatory}`, | ||
targetBinary(argv.targetBinary) |
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.
It could be transform as the line of the development option, to mantain uniformity of the codebase
it("return the app name", () => { | ||
const ret = appName("nameOfPackage", "platform"); | ||
it("return the app name [CASE: no android or ios code-push app name specified]", () => { | ||
const argv = { |
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.
It is an empty object, so it should close on the same line
@@ -41,13 +41,30 @@ const argv = yargs | |||
describe: "Check if you want your push is mandatory", | |||
type: "boolean" | |||
}) | |||
.option("na", { |
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.
na option is not an auto-explanatory alias, so it could be only nameAndroid without alias? The same is for ni and nameIos
I need some time to test if all works correctly. As soon as I have some time, I will do the check |
Cordova support: added the code-push release of a Cordova application, added "framework" argument in order to specify, "cordova" or "reactnative", default "reactnative" for backward compatibility. Implemented test of the new feature.
App Name: added arguments in order set the name of the registered code-push app, if not specified the behaviour is the same as before for backward compatibility. Implemented test of the new feature.
Update dependencies: updated the dependencies and refactored the "test" script in order to avoid deprecated message. For the "coverage" script I switched from isparta to nyc because isparta is no longer maintained.
Below the new "usage" image for the README.md :