Issue 29829605: Noissue - updating eslint dependency to v5 (Closed)
|
Created:
July 13, 2018, 4:04 p.m. by tlucas Modified:
Aug. 8, 2018, 10:08 a.m. Reviewers:
Sebastian Noack CC:
kzar, hub Visibility:
Public. |
DescriptionNoissue - updating eslint dependency to v5
A follow-up review to update eslint-config-eyeo's version will be uploaded, as soon as my access to npmjs.com was granted.
Patch Set 1 #
Total comments: 4
Patch Set 2 : #MessagesTotal messages: 6
https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... eslint-config-eyeo/package.json:26: "eslint": "5" As far as I understand, this would request ==5.0.0 (but there is already 5.1). I guess what we want here is ^5.1 (which means >=5.1,<5.2 or in other words give me the latest version compatible with 5.1). That way, if there will be any security (or otherwise critical) issue with 5.1.0 and they release 5.1.1 as a hotfix, we automatically get it. But an upgrade to 5.2 (or above) which might require further changes on our end would still need to be performed manually.
https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... eslint-config-eyeo/package.json:26: "eslint": "5" On 2018/07/13 16:13:47, Sebastian Noack wrote: > As far as I understand, this would request ==5.0.0 (but there is already 5.1). > > I guess what we want here is ^5.1 (which means >=5.1,<5.2 or in other words give > me the latest version compatible with 5.1). That way, if there will be any > security (or otherwise critical) issue with 5.1.0 and they release 5.1.1 as a > hotfix, we automatically get it. But an upgrade to 5.2 (or above) which might > require further changes on our end would still need to be performed manually. With npm's "semver" thingy in place, this would (confirmed locally) request the latest version of the major version 5 (in my case, it did install v5.1.0 [ by calling 'npm install eslint@5]) however, i wouldn't mind pinning this a bit tighter. But to specifically get only patches for 5.1 , we should go with 5.1.x.
https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... eslint-config-eyeo/package.json:26: "eslint": "5" On 2018/07/13 16:22:30, tlucas wrote: > On 2018/07/13 16:13:47, Sebastian Noack wrote: > > As far as I understand, this would request ==5.0.0 (but there is already 5.1). > > > > I guess what we want here is ^5.1 (which means >=5.1,<5.2 or in other words > give > > me the latest version compatible with 5.1). That way, if there will be any > > security (or otherwise critical) issue with 5.1.0 and they release 5.1.1 as a > > hotfix, we automatically get it. But an upgrade to 5.2 (or above) which might > > require further changes on our end would still need to be performed manually. > > With npm's "semver" thingy in place, this would (confirmed locally) request the > latest version of the major version 5 (in my case, it did install v5.1.0 [ by > calling 'npm install eslint@5]) however, i wouldn't mind pinning this a bit > tighter. But to specifically get only patches for 5.1 , we should go with 5.1.x. You seem to be right. I initially misunderstood the simplified documentation in the "dependencies" section for package.json [1]. However, in the detailed semver documentation [2] it clearly confirms what you are saying. I personally wouldn't mind a slightly broader requirement. But since we never tested it with ESLint 5.0, we might still want to go with ^5.1 after all. [1]: https://docs.npmjs.com/files/package.json#dependencies [2]: https://docs.npmjs.com/misc/semver
Patch Set 2: * Loosen the selected dependency to >=5.1 https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29829605/diff/29829606/eslint-config-eyeo/... eslint-config-eyeo/package.json:26: "eslint": "5" On 2018/07/13 17:06:57, Sebastian Noack wrote: > On 2018/07/13 16:22:30, tlucas wrote: > > On 2018/07/13 16:13:47, Sebastian Noack wrote: > > > As far as I understand, this would request ==5.0.0 (but there is already > 5.1). > > > > > > I guess what we want here is ^5.1 (which means >=5.1,<5.2 or in other words > > give > > > me the latest version compatible with 5.1). That way, if there will be any > > > security (or otherwise critical) issue with 5.1.0 and they release 5.1.1 as > a > > > hotfix, we automatically get it. But an upgrade to 5.2 (or above) which > might > > > require further changes on our end would still need to be performed > manually. > > > > With npm's "semver" thingy in place, this would (confirmed locally) request > the > > latest version of the major version 5 (in my case, it did install v5.1.0 [ by > > calling 'npm install eslint@5]) however, i wouldn't mind pinning this a bit > > tighter. But to specifically get only patches for 5.1 , we should go with > 5.1.x. > > You seem to be right. I initially misunderstood the simplified documentation in > the "dependencies" section for package.json [1]. However, in the detailed semver > documentation [2] it clearly confirms what you are saying. > > I personally wouldn't mind a slightly broader requirement. But since we never > tested it with ESLint 5.0, we might still want to go with ^5.1 after all. > > [1]: https://docs.npmjs.com/files/package.json#dependencies > [2]: https://docs.npmjs.com/misc/semver Acknowledged and done.
LGTM 
|
