-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Android Platform Release Preparation (Cordova 9) #612
Conversation
Codecov Report
@@ Coverage Diff @@
## master #612 +/- ##
=========================================
+ Coverage 62.21% 64.7% +2.48%
=========================================
Files 17 18 +1
Lines 1993 1816 -177
Branches 371 0 -371
=========================================
- Hits 1240 1175 -65
+ Misses 753 641 -112
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.
Looks good in general, wondering why we should get rid of npm-run-all
package.json
Outdated
@@ -48,7 +46,6 @@ | |||
"eslint-plugin-promise": "^4.0.1", | |||
"eslint-plugin-standard": "^4.0.0", | |||
"jasmine": "^3.3.1", | |||
"npm-run-all": "^4.1.5", |
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.
Why get rid of npm-run-all?
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.
@brodybits
I didn't think it was necessary to keep for only one use case.
There were two cases, but I don't see why second case was actually needed. Since we ESLint the entire directory and it passed, I didn't see a reason to lint the folder again with applied ignore-pattern
.
IF we still had a pattern matching requirement for the run-s
or run-p
and didn't have require order, maybe it would have been worth keeping.
In the end, I felt it became more of bloat-package since all it provided at this point was shorting one line by 10 chars.
run-s eslint unit-tests java-unit-tests e2e-tests
and
npm run eslint && npm run cover && npm run java-unit-tests
The package itself is 156kb in size but with all its dependencies it was about 2.4mb. Even though some of the sub-dependencies we may already have from other packages.
I felt it might be better to just cut/downsize.
If there is something I am missing, I can add it back but would also like to understand why.
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.
For dependencies
I would be strongly in favor of this kind of a reduction. But for devDependencies
which is build tooling I do not really see the benefit of getting rid of npm-run-all
. Even if we would only use it once I can imagine it would be nice if we could use it in more tasks someday.
The main thing is that I personally find it nicer if we don't have to do a lot of shell things in package.json
itself.
But I don't feel super strongly about one way or the other, leaving it up to your best judgement.
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.
Approved on my part.
While I would rather we keep npm-run-all
script in devDependencies I don't want this to be a blocker.
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.
👍 from me
Platforms affected
android
What does this PR do?
This PR contains final preparations for the Cordova 9 release goals.
See Cordova 9 Release Plan.
cordova-common@^3.1.0
eslint@^5.12.0
eslint-config-semistandard@^13.0.0
eslint-config-standard@^12.0.0
eslint-plugin-import@^2.14.0
eslint-plugin-node@^8.0.1
eslint-plugin-promise@^4.0.1
eslint-plugin-standard@^4.0.0
jasmine@^3.3.1
rewire@^4.0.1
elementtree@^0.1.7
nopt@^4.0.1
properties-parser@^0.3.1
nyc@^13.1.0
package.json
cleanupnpm-run-all
What testing has been done on this change?
npm t