Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

CB-14145 resolve npm audit issues in patch fix #281

Merged
merged 7 commits into from
Jul 11, 2018

Conversation

brodycj
Copy link

@brodycj brodycj commented Jul 6, 2018

Platforms affected

Windows

What does this PR do?

  • Set VERSION to 6.0.1-dev & update JS snapshot to version 6.0.1-dev via coho, using local coho with Improve patch release support cordova-coho#176 for patch release support (on mac due to some issues with updating JavaScript on Windows)
  • Update cordova-common to 2.2.5, pinned in this patch fix, to resolve the npm audit issues
  • pin other dependencies in package.json in this patch fix
  • completely reinstall node_modules (ignoring node_modules/.bin) using the following command on npm@6.1.0: npm install --only=production
  • Update cordova.js from cordova-js@4.2.4 with the following changes, using local coho with Improve patch release support cordova-coho#176 for patch release support (on mac due to some issues with updating JavaScript on Windows):
    • CB-9366 log error.stack
  • update bundledDependencies to support deprecated Node.js 4 in this patch fix
  • add Node.js 8 & 10 to AppVeyor CI & Travis CI in this patch fix
  • add blank lines to .travis.yml in this patch fix
  • RELEASENOTES.md 6.0.0 fixes
    • remove extra info (now obsolete)
    • note cordova.js update from cordova-js@4.2.2
    • other minor fixes

What testing has been done on this change?

  • able to build and run Cordova Windows app when using deprecated Node.js version 4 without ugly engine error warnings
  • able to build and run Cordova Windows app on Node.js version 8
  • npm audit (using npm@6.1.0) shows 0 vulnerabilities

CI testing done:

  • verify that npm test and other items succeed on AppVeyor CI
  • verify that build tests items succeed on Travis CI

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. (with some exceptions)
  • Added automated test coverage as appropriate for this change.

@brodycj brodycj requested review from janpio, dpogue and raphinesse July 8, 2018 22:30
@janpio
Copy link
Member

janpio commented Jul 9, 2018

pin other dependencies in package.json in this patch fix

Why?

update bundledDependencies to support deprecated Node.js 4 in this patch fix

Why are so many additional libraries now listed there?

Commit: .gitignore ignore package-lock.json in 6.0.x

was this somewhere decided to how Cordova should handle package-lock.json?

Commit: CB-14145 remove node_modules before patch fix

Why?

Several commits: "... in 6.0.x"

What does this mean? Won't these commits get merged to master as well?

Is it correct that RELEASENOTES don't have a 6.0.1 entry here?

@brodycj
Copy link
Author

brodycj commented Jul 9, 2018

pin other dependencies in package.json in this patch fix

Why?

Keep npm install behavior as predictable as possible.

update bundledDependencies to support deprecated Node.js 4 in this patch fix

Why are so many additional libraries now listed there?

With node_modules installed by newer version of npm (comes with non-deprecated version of Node.js), additional libraries need to be listed to work on Node.js 4. We know that Node.js 4 is deprecated but should not be dropped in a patch release:-(

Commit: .gitignore ignore package-lock.json in 6.0.x

was this somewhere decided to how Cordova should handle package-lock.json?

I think this was discussed in document on dev list for next major release (not sure). But I think we do not want to introduce this file in patch release, that is why I added it to .gitignore.

Commit: CB-14145 remove node_modules before patch fix

Why?

A combination of updated dependencies and npm from non-deprecated version of Node.js results in such a massive change to node_modules that it seems cleanest to remove old node_modules before making the update.

Several commits: "... in 6.0.x"

What does this mean? Won't these commits get merged to master as well?

The changes proposed here are tailored specifically to the patch release in the 6.0.x branch. A number of changes are needed in node_modules since we should not drop Node.js 4 in a patch release. But I think we do not want all of these changes in the master branch.

I think we want to take a cleaner approach in the master branch: drop Node.js 4 support, remove committed node_modules, and target the next major release.

I would be happy to add a note to some of the commits with the reason why we do not want them in the master branch.

Is it correct that RELEASENOTES don't have a 6.0.1 entry here?

Yes I did not do that part yet. (I think it should be in another JIRA task according to https://github.com/apache/cordova-coho/blob/master/docs/platforms-release-process.md.)

@janpio
Copy link
Member

janpio commented Jul 9, 2018

Ok, to be honest I still don't really understand what is going on here, but if you are confident that this won't break anything please go ahead - I don't want to block any cleanup, and there should be no negative impact because of these changes.

Christopher J. Brody added 2 commits July 10, 2018 10:38
Christopher J. Brody added 5 commits July 10, 2018 12:15
@brodycj brodycj closed this Jul 10, 2018
@brodycj brodycj deleted the cb-14145-patch branch July 10, 2018 16:16
@brodycj brodycj restored the cb-14145-patch branch July 10, 2018 16:16
@brodycj
Copy link
Author

brodycj commented Jul 10, 2018

Reopening, will remove appveyor.yml updates from patch fix, apologies for the confusion.

@brodycj brodycj reopened this Jul 10, 2018
@janpio
Copy link
Member

janpio commented Jul 10, 2018

Reopening, will remove appveyor.yml updates from patch fix, apologies for the confusion.

Why? Who was confused? I have 0 context what is going on here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants