BREAKING: Bump all ESLint dependencies #351
Conversation
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. What is shell access?This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code. Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
| "eslint-plugin-import": "~2.26.0", | ||
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", |
There was a problem hiding this comment.
I'm not sure why we used this range before, but I replaced it with the latest (Node.js 16-compatible) version. I can revert if there's a good reason to have this range.
cc @legobeat
There was a problem hiding this comment.
| 'max-statements-per-line': [ | ||
| 'error', | ||
| { | ||
| max: 1, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Our validation script was complaining about this being handled by Prettier.
| 'jsdoc/tag-lines': [ | ||
| 'error', | ||
| 'any', | ||
| { | ||
| startLines: 1, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
AFAIK this matches the previous behaviour, where one line between the description and the first tag is required.
| // Should be disabled per Prettier | ||
| '@typescript-eslint/no-extra-semi': 'off', |
There was a problem hiding this comment.
Our validation script was complaining about this being handled by Prettier.
| @@ -1,3 +1,6 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "strictNullChecks": true | |||
There was a problem hiding this comment.
This is now required by @typescript-eslint/eslint-plugin when type checking is enabled. I think most of our repos use strict which also enables this anyway.
Mrtenz
changed the title
|
Thanks so much for doing this work, this is super helpful. I'll try to take a look soon. |
Mrtenz
force-pushed
the
mrtenz/bump-dependencies
branch
from
69a7ea5
to
4eb7750
Compare
|
Bumping to Prettier v3 will require some more changes, so we may want to do that separately.
|
| "eslint-plugin-import": "~2.26.0", | ||
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "eslint-plugin-jsdoc": "^47.0.2", |
There was a problem hiding this comment.
For peerDeps, what do you think about being a bit more lenient with supported versions? Motivation being to reduce the amount and risk of update-hell where all packages need to be updated in lockstep and there may be external conflicting peerDeps from other packages at play.
So something like
| "eslint-plugin-jsdoc": "^47.0.2", | |
| "eslint-plugin-jsdoc": "^43.0.7 || ^47.0.2", |
or
| "eslint-plugin-jsdoc": "^47.0.2", | |
| "eslint-plugin-jsdoc": ">=43.0.7 <48", |
would allow users to upgrade in a more incremental fashion. Especially when new versions break engines or other peerDeps, this can make a big difference.
There was a problem hiding this comment.
I don't think we need to bend over backward when it comes to peer dependencies like this. If we're upgrading ESLint packages and the new version of this package ends up adding a new rule and we don't want to deal with it, we can follow the approach we take now, which is to disable it temporarily and come back in a new PR to re-enable it. Otherwise, we fix lint violations along with the upgrade.
If we continue to use a version range like this, it's only going to prompt the question "why", and anyone who wants an answer will have to dig through pull requests to find the context all over again.
There was a problem hiding this comment.
I realize that I was a bit dismissive here. I've looked back over the notes you left on eslint-plugin-jsdoc, @legobeat, specifically this comment. Thanks for summarizing the issues there.
To give more context to my previous comment, our ESLint configurations rely on specific versions of plugins/presets, since the rules we use come from those versions, and so in general I think it's okay if clients usually need to update a slew of dependencies whenever they want to use a new version of @metamask/eslint-config. Also, if we release a major version (such as we would be with this PR) then we are communicating that clients may need to do extra work, such as fixing lint violations, or making other changes, if they choose to upgrade. Obviously we don't want this to be a painful process, but a little work is okay, I think. That's where I was coming from.
That said, I am slowly realizing that this plugin, eslint-plugin-jsdoc is badly managed. They have no changelog file, they've made breaking changes (such as bumping the Node version) in non-major releases, and the notes for releases prior to 45.0.0 are entirely unreadable. And now with 48.x they've moved to ESM and in the process have most likely broken the CJS build (if Are the Types Wrong? is to be trusted). So in hindsight it's no surprise that we've had a frustrating experience with this package, and apparently, we will continue to do so in future versions.
So, my response in the previous comment was a bit mistargeted, and I agree that dealing with eslint-plugin-jsdoc doesn't seem like something we ought to foist on clients.
If we feel that remaining with 43.0.7 would prevent frustration, then my vote would be to use "eslint-plugin-jsdoc": ">=43.0.7 <48".
There was a problem hiding this comment.
"eslint-plugin-jsdoc": ">=43.0.7 <48" SGTM!
And while, yes, this particular package has been especially cumbersome, I think we might want to be less aggressive in removing support for old peerDeps in this package as we introduce support for newer version to avoid the general problem.
If there is a practical reason to break an old version, of course we can do so, but I don't think we should default to only ever supporting the latest possible version at whatever point we're updating a peerDependency.
(This reasoning only applies to peerDependencies)
There was a problem hiding this comment.
Okay. Sounds like a guideline that we need to create. I'll make a note to record this somewhere, perhaps the contributor-docs, so we can remind ourselves about it.
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "eslint-plugin-jsdoc": "^47.0.2", | ||
| "eslint-plugin-prettier": "^5.1.3", |
There was a problem hiding this comment.
Along the lines of above, is this doable without too much hassle?
| "eslint-plugin-prettier": "^5.1.3", | |
| "eslint-plugin-prettier": "^4.2.1 || ^5.1.3", |
There was a problem hiding this comment.
eslint-plugin-prettier 5.0.0 places a peer dependency on ESLint 8, which we already use. Going through the changelog there don't seem to be any real breaking changes to this package in 5.0.0 and beyond, so forcing consumers to bump this doesn't seem like a big ask. This seems contingent on upgrading to Prettier 3, however.
There was a problem hiding this comment.
If there's nothing major, why force users to update rather than maintaining compatibility with eslint-plugin-prettier v4?
There was a problem hiding this comment.
I added a note at the end, but I take this back — the major change here is a reliance on Prettier 3. If we want to not make that change yet, then perhaps we ought to roll this back.
There was a problem hiding this comment.
To answer your comment though:
If there's nothing major, why force users to update rather than maintaining compatibility with
eslint-plugin-prettierv4?
In this case, if there is nothing that clients would have to do in upgrading to Prettier 3, then why not ask users them to upgrade? I would think that we would want our packages to be on the latest versions of the tools we use, no?
There was a problem hiding this comment.
Asking is one thing, disallowing is another. Consider also the case of prettier here. It's not even clear if the upgrade to v3 is feasible at this point. So we're cautious about forcing the upgrade. Why not support both v2 and v3 concurrently until such a point that we don't rely on v2 in any meaningful sense anymore, or at least have used the new version of prettier with the eslint configs in a few packages and see it work out?
Why force all updates in lockstep by default?
| "eslint-plugin-promise": "^6.1.1", | ||
| "prettier": "^2.7.1" | ||
| "prettier": "^3.2.5" |
There was a problem hiding this comment.
https://github.com/MetaMask/eslint-config/pull/351/files#r1566550269
(GH won't let me do multiline-suggestion here)
| "prettier": "^3.2.5" | |
| "prettier": "^2.8.7 || ^3.2.5" |
There was a problem hiding this comment.
@Mrtenz You said:
Bumping to Prettier v3 will require some more changes, so we may want to do that separately.
So do we still want to do this?
| "eslint-plugin-import": "~2.26.0", | ||
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "eslint-plugin-jsdoc": "^47.0.2", |
There was a problem hiding this comment.
I don't think we need to bend over backward when it comes to peer dependencies like this. If we're upgrading ESLint packages and the new version of this package ends up adding a new rule and we don't want to deal with it, we can follow the approach we take now, which is to disable it temporarily and come back in a new PR to re-enable it. Otherwise, we fix lint violations along with the upgrade.
If we continue to use a version range like this, it's only going to prompt the question "why", and anyone who wants an answer will have to dig through pull requests to find the context all over again.
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "eslint-plugin-jsdoc": "^47.0.2", | ||
| "eslint-plugin-prettier": "^5.1.3", |
There was a problem hiding this comment.
eslint-plugin-prettier 5.0.0 places a peer dependency on ESLint 8, which we already use. Going through the changelog there don't seem to be any real breaking changes to this package in 5.0.0 and beyond, so forcing consumers to bump this doesn't seem like a big ask. This seems contingent on upgrading to Prettier 3, however.
| "eslint-plugin-promise": "^6.1.1", | ||
| "prettier": "^2.7.1" | ||
| "prettier": "^3.2.5" |
There was a problem hiding this comment.
@Mrtenz You said:
Bumping to Prettier v3 will require some more changes, so we may want to do that separately.
So do we still want to do this?
This bumps all ESLint and related dependencies to the latest version which has support for Node 16. When we drop support for Node 16, some dependencies can be bumped again.
The only exception is
eslint-plugin-import, which has a regression since>=27.0.0. I'm planning to swap it out witheslint-plugin-import-x(a fork ofeslint-plugin-import) since it has significantly better performance regardless.Closes #349.
Closes #338.
Breaking changes
@typescript-eslint/eslint-pluginnow requires a boolean forparserOptions.project, instead of a path totsconfig.json.@typescript-eslint/eslint-pluginnow requiresstrictNullChecksto be enabled.