Suggestion: Allow only patch updates on package.json #11493

luixxiul · 2017-10-12T18:16:16Z

This PR replaces carets with tildes to allow only patch updates on package.json.

Because of minor updates a nasty bug like #11454 and #10029 sometimes happens, which is hard to find what introduced them (there seems to be almost no way of detecting it automatically). The thing is that semver is just a norm and developers can regard an update as a patch mistakenly, where it actually is a minor update.

Forcing only patch updates is one step to remove semver, which introduces a security risk as well. IMHO, major / minor updates should be done via PR to merge the change after it is verified via Travis tests result that nothing should get corrupted due to that update, locally and remotely.

This PR is based on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16 which you should get with npm list on 7de1673. I added comments to each update, referring to that gist, to make it easy to review.

As long as the packages.json is not manually updated, this PR should be valid.

Closes #11458

Auditors:

Test Plan:

npm install

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

Closes #11458 Minor update sometimes causes a nasty bug like #11454 and #10029 Auditors: Test Plan: 1. npm install

luixxiul · 2017-10-12T18:17:02Z

package.json

@@ -84,111 +84,111 @@
  "homepage": "https://www.brave.com/",
  "dependencies": {
    "acorn": "3.2.0",
-    "ad-block": "^3.0.5",
+    "ad-block": "~3.0.5",


ad-block@3.0.5 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L3

luixxiul · 2017-10-12T18:17:56Z

package.json

-    "bat-client": "^1.0.5",
-    "bat-publisher": "^1.0.2",
-    "bignumber.js": "^4.0.4",
+    "async": "~2.5.0",


async@2.5.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L58

luixxiul · 2017-10-12T18:18:24Z

package.json

-    "bat-publisher": "^1.0.2",
-    "bignumber.js": "^4.0.4",
+    "async": "~2.5.0",
+    "bat-balance": "~1.0.1",


bat-balance@1.0.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L325

luixxiul · 2017-10-12T18:18:44Z

package.json

-    "bignumber.js": "^4.0.4",
+    "async": "~2.5.0",
+    "bat-balance": "~1.0.1",
+    "bat-client": "~1.0.5",


bat-client@1.0.5 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L375

luixxiul · 2017-10-12T18:19:17Z

package.json

+    "async": "~2.5.0",
+    "bat-balance": "~1.0.1",
+    "bat-client": "~1.0.5",
+    "bat-publisher": "~1.0.2",


bat-publisher@1.0.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L772

luixxiul · 2017-10-12T18:19:43Z

package.json

+    "bat-balance": "~1.0.1",
+    "bat-client": "~1.0.5",
+    "bat-publisher": "~1.0.2",
+    "bignumber.js": "~4.0.4",


bignumber.js@4.0.4 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L794

luixxiul · 2017-10-12T18:20:07Z

package.json

-    "clipboard-copy": "^1.0.0",
-    "compare-versions": "^3.0.1",
-    "electron-localshortcut": "^0.6.0",
+    "clipboard-copy": "~1.2.0",


clipboard-copy@1.2.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L827

luixxiul · 2017-10-12T18:20:27Z

package.json

-    "compare-versions": "^3.0.1",
-    "electron-localshortcut": "^0.6.0",
+    "clipboard-copy": "~1.2.0",
+    "compare-versions": "~3.1.0",


compare-versions@3.1.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L827

luixxiul · 2017-10-12T18:20:54Z

package.json

-    "electron-localshortcut": "^0.6.0",
+    "clipboard-copy": "~1.2.0",
+    "compare-versions": "~3.1.0",
+    "electron-localshortcut": "~0.6.1",


electron-localshortcut@0.6.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1386

luixxiul · 2017-10-12T18:21:10Z

package.json

    "electron-squirrel-startup": "brave/electron-squirrel-startup",
-    "emoji-regex": "^6.5.1",
+    "emoji-regex": "~6.5.1",


emoji-regex@6.5.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1457

luixxiul · 2017-10-12T18:21:25Z

package.json

    "file-loader": "0.11.2",
-    "font-awesome": "^4.5.0",
+    "font-awesome": "~4.7.0",


font-awesome@4.7.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1541

luixxiul · 2017-10-12T18:22:10Z

package.json

-    "fs-extra": "^2.1.2",
-    "immutable": "^3.7.5",
-    "immutablediff": "^0.4.2",
+    "fs-extra": "~2.1.2",


fs-extra@2.1.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1566

luixxiul · 2017-10-12T18:25:03Z

package.json

-    "immutable": "^3.7.5",
-    "immutablediff": "^0.4.2",
+    "fs-extra": "~2.1.2",
+    "immutable": "~3.8.1",


immutable@3.8.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1736

luixxiul · 2017-10-12T18:25:14Z

package.json

-    "immutablediff": "^0.4.2",
+    "fs-extra": "~2.1.2",
+    "immutable": "~3.8.1",
+    "immutablediff": "~0.4.4",


immutablediff@0.4.4 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1737

luixxiul · 2017-10-12T18:25:34Z

package.json

    "immutablepatch": "brave/immutable-js-patch",
-    "l20n": "^3.5.1",
-    "lru-cache": "^1.0.0",
+    "l20n": "~3.5.1",


l20n@3.5.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1849

luixxiul · 2017-10-12T18:25:58Z

package.json

-    "l20n": "^3.5.1",
-    "lru-cache": "^1.0.0",
+    "l20n": "~3.5.1",
+    "lru-cache": "~1.1.1",


lru-cache@1.1.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1915

luixxiul · 2017-10-12T18:26:21Z

package.json

-    "prettier-bytes": "^1.0.3",
-    "prop-types": "^15.5.6",
-    "punycode": "^2.0.0",
+    "niceware": "~1.0.4",


niceware@1.0.4 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1966

luixxiul · 2017-10-12T18:26:38Z

package.json

-    "prop-types": "^15.5.6",
-    "punycode": "^2.0.0",
+    "niceware": "~1.0.4",
+    "parse-torrent": "~5.8.3",


parse-torrent@5.8.3 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2208

luixxiul · 2017-10-12T18:27:01Z

package.json

-    "punycode": "^2.0.0",
+    "niceware": "~1.0.4",
+    "parse-torrent": "~5.8.3",
+    "prettier-bytes": "~1.0.4",


prettier-bytes@1.0.4 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2240

luixxiul · 2017-10-12T18:27:29Z

package.json

+    "niceware": "~1.0.4",
+    "parse-torrent": "~5.8.3",
+    "prettier-bytes": "~1.0.4",
+    "prop-types": "~15.6.0",


prop-types@15.6.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2241

luixxiul · 2017-10-12T18:28:10Z

package.json

+    "parse-torrent": "~5.8.3",
+    "prettier-bytes": "~1.0.4",
+    "prop-types": "~15.6.0",
+    "punycode": "~2.1.0",


punycode@2.1.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2258

luixxiul · 2017-10-12T18:29:36Z

package.json

-    "react-dom": "^15.5.4",
-    "string.prototype.endswith": "^0.2.0",
-    "string.prototype.startswith": "^0.2.0",
+    "react": "~15.6.2",


react@15.6.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2266

luixxiul · 2017-10-12T18:29:49Z

package.json

-    "string.prototype.endswith": "^0.2.0",
-    "string.prototype.startswith": "^0.2.0",
+    "react": "~15.6.2",
+    "react-dnd": "~2.5.1",


react-dnd@2.5.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2279

luixxiul · 2017-10-12T18:30:06Z

package.json

-    "string.prototype.startswith": "^0.2.0",
+    "react": "~15.6.2",
+    "react-dnd": "~2.5.1",
+    "react-dnd-html5-backend": "~2.5.1",


react-dnd-html5-backend@2.5.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2294

luixxiul · 2017-10-12T18:30:18Z

package.json

+    "react": "~15.6.2",
+    "react-dnd": "~2.5.1",
+    "react-dnd-html5-backend": "~2.5.1",
+    "react-dom": "~15.6.2",


react-dom@15.6.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2296

luixxiul · 2017-10-12T18:48:32Z

package.json

+    "mockery": "~2.1.0",
+    "muon-winstaller": "~2.5.5",
+    "ncp": "~2.0.0",
+    "node-gyp": "~3.6.2",


node-gyp@3.6.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L1970

luixxiul · 2017-10-12T18:48:54Z

package.json

    "node-libs-browser": "~2.0.0",
-    "node-static": "^0.7.7",
-    "nsp": "^2.2.0",
+    "node-static": "~0.7.10",


node-static@0.7.10 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2152

luixxiul · 2017-10-12T18:49:12Z

package.json

-    "node-static": "^0.7.7",
-    "nsp": "^2.2.0",
+    "node-static": "~0.7.10",
+    "nsp": "~2.8.0",


nsp@2.8.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2158

luixxiul · 2017-10-12T18:49:30Z

package.json

-    "react-test-renderer": "^15.5.4",
-    "request": "^2.81.0",
-    "sinon": "^1.17.6",
+    "pre-push": "~0.1.1",


pre-push@0.1.1 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2238

luixxiul · 2017-10-12T18:49:51Z

package.json

-    "request": "^2.81.0",
-    "sinon": "^1.17.6",
+    "pre-push": "~0.1.1",
+    "react-addons-perf": "~15.4.2",


react-addons-perf@15.4.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2275

luixxiul · 2017-10-12T18:50:10Z

package.json

-    "sinon": "^1.17.6",
+    "pre-push": "~0.1.1",
+    "react-addons-perf": "~15.4.2",
+    "react-addons-test-utils": "~15.6.2",


react-addons-test-utils@15.6.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2278

luixxiul · 2017-10-12T18:50:34Z

package.json

+    "pre-push": "~0.1.1",
+    "react-addons-perf": "~15.4.2",
+    "react-addons-test-utils": "~15.6.2",
+    "react-test-renderer": "~15.6.2",


react-test-renderer@15.6.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2301

luixxiul · 2017-10-12T18:51:28Z

package.json

+    "react-addons-perf": "~15.4.2",
+    "react-addons-test-utils": "~15.6.2",
+    "react-test-renderer": "~15.6.2",
+    "request": "~2.82.0",


request@2.82.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2304

luixxiul · 2017-10-12T18:51:52Z

package.json

+    "react-addons-test-utils": "~15.6.2",
+    "react-test-renderer": "~15.6.2",
+    "request": "~2.82.0",
+    "sinon": "~1.17.7",


sinon@1.17.7 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2365

luixxiul · 2017-10-12T18:52:34Z

package.json

    "spectron": "brave/spectron#chromium60",
    "standard": "9.0.0",
    "style-loader": "~0.18.2",
-    "uglifyjs-webpack-plugin": "^1.0.0-beta.2",
-    "uuid": "^3.0.1",
+    "uglifyjs-webpack-plugin": "~1.0.0-beta.2",


uglifyjs-webpack-plugin@1.0.0-beta.2 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2743

luixxiul · 2017-10-12T18:52:49Z

package.json

-    "uglifyjs-webpack-plugin": "^1.0.0-beta.2",
-    "uuid": "^3.0.1",
+    "uglifyjs-webpack-plugin": "~1.0.0-beta.2",
+    "uuid": "~3.1.0",


uuid@3.1.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L2873

luixxiul · 2017-10-12T18:53:52Z

package.json

    "webdriverio": "4.7.1",
    "webpack": "~3.4.1",
    "webpack-dev-server": "~2.6.1",
-    "webpack-notifier": "^1.2.1",
-    "xml2js": "^0.4.15"
+    "webpack-notifier": "~1.5.0",


webpack-notifier@1.5.0 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L3646

luixxiul · 2017-10-12T18:54:09Z

package.json

-    "webpack-notifier": "^1.2.1",
-    "xml2js": "^0.4.15"
+    "webpack-notifier": "~1.5.0",
+    "xml2js": "~0.4.19"


xml2js@0.4.19 on https://gist.github.com/luixxiul/1cf8689da05e509d45176588730e5a16#file-npm-txt-L3994

petemill · 2017-10-19T05:23:38Z

If we do this, then we could use an automated service like https://greenkeeper.io/ which monitors npm dependencies, then sends PRs when each npm dependency has a new minor / major version, allowing our tests to run with only that update

evq · 2017-10-19T18:30:10Z

I'm not sure this will have much of an effect, we have a package-lock.json and my understanding is that this actually freezes to an exact version including patch. So it should be possible to determine the source (commit) that introduced any breaking package upgrade.

I think package-lock.json changes can be quite hard to read manually, so maybe we need to look for some automation so changes can be reviewed in a concise format?

luixxiul · 2017-10-19T18:51:32Z

my understanding is that this actually freezes to an exact version including patch.

I understood just now what package-lock.json serves for: https://stackoverflow.com/a/44297998

Still I am not sure why #11454 (PR #11453) has happened then. If the package were not updated automatically, the tests would have not broken on Travis, haven't they?

evq · 2017-10-19T20:44:25Z

oh wow. I looked into this a little more (thanks for the stackoverflow link, found this via a comment) and it turns out there is an NPM issue discussing a change around 5.3.0 where package-lock.json wasn't pinning an exact version. A comment on that issue seems to indicate that the latest version has the behavior one would expect, namely:

If you have a package.json and you run npm i we generate a package-lock.json from it.

If you run npm i against that package.json and package-lock.json, the latter will never be updated, even if the package.json would be happy with newer versions.

If you manually edit your package.json to have different ranges and run npm i and those ranges aren't compatible with your package-lock.json then the latter will be updated with version that are compatible with your package.json. Further runs of npm i will be as with 2 above.

npm/npm#17979 (comment)

Perhaps this led to the issues you mentioned?

I just tested a clean npm install with npm 5.5.1, and there were no version changes to package-lock.json. We could set the engine version of npm in package.json to 5.5.1 or another newer version.

luixxiul · 2017-10-29T11:58:00Z

Closing for now to see if another issue would happen after upgrading npm version.

bsclifton

I think closing this is the correct action

We should always be using the package-lock.json file. We should never be removing this file... in the cases where we need to update a specific package, you can simply npm install package_name@new_version_here --save and it'll update the package, update package.json, and also update the package-lock.json

This PR wouldn't be adding any value, unfortunately. It would make a difference if folks deleted the package-lock.json when upgrading a package... but that should not be happening

Allow only patch updates on package.json

cd82309

Closes #11458 Minor update sometimes causes a nasty bug like #11454 and #10029 Auditors: Test Plan: 1. npm install

luixxiul added dev-setup suggestion labels Oct 12, 2017

luixxiul self-assigned this Oct 12, 2017

luixxiul added the PR/work-in-progress ⚒ label Oct 12, 2017

luixxiul commented Oct 12, 2017

View reviewed changes

luixxiul requested review from bbondy and evq October 12, 2017 18:55

luixxiul removed the PR/work-in-progress ⚒ label Oct 12, 2017

cezaraugusto added the PR/pending-review label Oct 14, 2017

luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Oct 19, 2017

luixxiul closed this Oct 29, 2017

luixxiul removed needs-info Another team member needs information from the PR/issue opener. PR/pending-review labels Oct 29, 2017

luixxiul removed request for bbondy and evq October 29, 2017 11:58

bsclifton reviewed Oct 30, 2017

View reviewed changes

ayumi mentioned this pull request Nov 12, 2017

Use package-lock.json in Travis #11911

Closed

Suggestion: Allow only patch updates on package.json #11493

Suggestion: Allow only patch updates on package.json #11493

Conversation

luixxiul commented Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Oct 19, 2017

evq commented Oct 19, 2017

luixxiul commented Oct 19, 2017 • edited Loading

evq commented Oct 19, 2017

luixxiul commented Oct 29, 2017

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Oct 12, 2017 •

edited

Loading

luixxiul commented Oct 19, 2017 •

edited

Loading