Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

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

Merged
merged 21 commits into from
Jul 30, 2018

Conversation

brodycj
Copy link

@brodycj brodycj commented Jul 6, 2018

Platforms affected

mac OS ("osx")

What does this PR do?

  • Set VERSION to 4.0.2-dev & update JS snapshot to version 4.0.2-dev via coho, using local coho with Improve patch release support cordova-coho#176 for patch release support (another workaround fix pushed to Improve patch release support cordova-coho#176 was needed to get the JS snapshot to work with old version of cordova-js). Note that the indentation of package.json was changed by use of a recent version of coho.
  • Update the following dependencies, pinned in this patch fix, to resolve the npm audit issues:
    • cordova-common@2.2.5
    • xcode@1.0.0 xcode@0.9.3 (same as cordova-ios)
  • explicit plist@2.1.0 dependency in this patch fix (wanted since plist is explicitly used by code in bin ref: [WIP] Major release TODO WIP #49)
  • pin other external dependencies in this patch fix
  • use jasmine-node@^1.5.0 in devDependencies to resolve npm audit warning in this patch fix
  • completely reinstall node_modules in this patch fix (ignoring node_modules/.bin) using the following command on npm@6.1.0: npm install --only=production:
  • Update CordovaLib/cordova.js from cordova-js versions 4.2.0, 4.2.2, and 4.2.4
  • update bundledDependencies to support deprecated Node.js 4 in this patch fix
  • remove extra devDependencies: coffee-script, nodeunit, uncrustify ([WIP] Major release TODO WIP #49)
  • fixes ported from master:
    • CB-12762: package.json reference repo on github
    • migrate jshint -> eslint, with some more eslint rules turned off in this patch fix
  • add line spacing to .eslintrc.yml ([WIP] Major release TODO WIP #49)
  • enable eslint in tests/spec (with some eslint rules turned off in tests/spec for now) and resolve eslint issues in tests/spec ([WIP] Major release TODO WIP #49)

What testing has been done on this change?

  • npm audit with npm@6.1.0 npm@6.2.0 (latest release) shows 0 vulnerabilities
  • npm outdated shows no red entries
  • npm test succeeds locally
  • cordova platform add brodybits/cordova-osx#cb-14145-patch to new Cordova project and then cordova run osx succeeds on deprecated Node.js 4 (npm@2.15.11) and Node.js 8 (npm@5.6.0)
  • cordova platform add brodybits/cordova-osx#cb-14145-patch to new Cordova project and then run from Xcode succeeds on deprecated Node.js 4 (npm@2.15.11)
  • cordova platform add brodybits/cordova-osx#cb-14145-patch to new Cordova project and then cordova build osx succeeds on deprecated Node.js 4 (npm@2.15.11)
  • TODO: check that Travis CI which includes npm test task passes (skipping this step since other changes would be needed for this project to pass its tests 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.

Christopher J. Brody and others added 8 commits July 6, 2018 16:46
* CB-3785: enable EventListener interface support
* CB-3785: Channel.prototype.subscribe to support EventListener interface
* CB-9967 deleted legacy platform specific files from cordova-js
* CB-11522 [windows] Make cordova-js handle 'unknown' type
* CB-11522 Make utils.clone handle properties gracefully
update from cordova-js@4.2.4
- coffee-script
- nodeunit
- uncrustify

and move tmp to end of the list
Co-authored-by: Audrey So <audreyso@apache.org>
Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
in devDependencies to resolve some npm audit warnings
audreyso and others added 7 commits July 8, 2018 15:50
resolves some npm audit warnings

with changes by @brodybits (Christopher J. Brody):
- .eslintrc.yml turn off more eslint rules in 4.0.x patch release
- package.json skip eslint on tests
  (disabled in .eslintignore)

Co-authored-by: Audrey So <audreyso@apache.org>
Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
with some eslint rules turned off
Outdated Cordova dependencies not included:
- cordova-common
- xcode (from apache/cordova-node-xcode)
remove:
- /node_modules

add:
- node_modules/.bin
- package-lock.json
- tests/cdv-test-project/node_modules/*
- tests/cdv-test-project/package.json
- lodash-node
- lodash
- os-homedir
- os-tmpdir
- osenv
Copy link
Contributor

@purplecabbage purplecabbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sounds good to me. The changed file list was so large I was not able to see it all, but it seems a large amount of this is just node_modules updates. Any chance we can stop bundling to clean some of this up?

@brodycj
Copy link
Author

brodycj commented Jul 9, 2018

Any chance we can stop bundling to clean some of this up?

Bundling is done to support Node.js 4, which we know is deprecated but cannot be dropped in a patch release. Hope we can remove the bundled node_modules from master in the near future for major release.

For this kind of a change I would generally suggest checking each of the individual commits.

Thanks @purplecabbage!

@brodycj
Copy link
Author

brodycj commented Jul 10, 2018

I would like to make one more update to include xcode@1.0.1 with [dev] audit fixes in apache/cordova-node-xcode#10. Apologies for any possible confusion.

@tripodsan
Copy link
Contributor

can't we fix this together with #48 and #49 to get rid of the bundled node_modules ?

@dpogue
Copy link
Member

dpogue commented Jul 10, 2018

We will remove the bundled node_modules in the next major version, but we can't do that in a patch release because it breaks compatibility with older versions of the cordova tooling.

Christopher J. Brody added 4 commits July 29, 2018 18:53
- cordova-common@2.2.5
- xcode@0.9.3 (same as cordova-ios@4.5.5)
in dependencies

(explicitly used by some scripts in bin/templates/scripts/cordova/lib)
installed by using the following command with npm@6.2.0:

    npm i --only=production

with the following artifacts ignored:

    node_modules/.bin/*
    package-lock.json
(needed to support the deprecated Node.js 4 version)
Christopher J. Brody added 2 commits July 29, 2018 21:52
Needed to clear the red highlights in npm outdated --depth=0
@brodycj brodycj merged commit 08c5083 into apache:4.0.x Jul 30, 2018
@brodycj brodycj deleted the cb-14145-patch branch July 30, 2018 03:00
@brodycj
Copy link
Author

brodycj commented Jul 30, 2018

Merged with a couple more updates:

  • using xcode@0.9.3 (same as cordova-ios@4.5.5)
  • resolve eslint issues in tests/spec

Also checked that npm outdated shows no red entries, other test items repeated.

See updated description for more details.

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.

5 participants