typescript: add support for typescript 5.0.x, 5.1.x#288
typescript: add support for typescript 5.0.x, 5.1.x#288legobeat merged 9 commits intoMetaMask:mainfrom
Conversation
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
Did you verify that the ESLint plugin works with TypeScript 5? Might have to bump that too. |
|
Good timing. I was just thinking about this yesterday! |
Seems fine (and tests are now running on ts5) but it did seem warranted to bump Also lifted the |
packages/base/package.json
Outdated
| "eslint-config-prettier": "^8.5.0", | ||
| "eslint-plugin-import": "^2.26.0", | ||
| "eslint-plugin-jsdoc": "^39.6.2", | ||
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", |
There was a problem hiding this comment.
For background of motivation of the specific version, see #290.
- Minimize upgrade overhead (keep existing
^39.6.2) - Maintenance fixes for existing users without breaking rules (
^41) - Forwards-compatibility with new rules while allowing for node 14 (
^43.0.7)
Open to holding off on introducing 43 here if considered premature or out of scope.
There was a problem hiding this comment.
Hmm. Wouldn't using this with v43 cause the error that #290 is meant to resolve? Perhaps we should wait for that PR before introducing this.
There was a problem hiding this comment.
In order to unblock this and break the relationship between the two PRs, I'm dropping ^43.0.7 from this one.
packages/typescript/package.json
Outdated
| "@typescript-eslint/parser": "^5.42.1", | ||
| "eslint": "^8.27.0", | ||
| "typescript": "~4.8.4" | ||
| "typescript": "~4.8.4 || ^5.0.0" |
There was a problem hiding this comment.
We had been intentionally using ~ ranges for TypeScript to prevent it from being used on TypeScript versions that may not be compatible with the plugin we're using. i.e. to prevent this error:
WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.
Without this, it's easy to upgrade and not notice any incompatibility until you run the linter locally and see that message.
There was a problem hiding this comment.
I agree. It's annoying that @typescript-eslint isn't automatically compatible with future versions of TypeScript, but I think it's better for our projects to not get updated to a new version of TypeScript prematurely than it is to have to keep the TypeScript versions manually updated. What do you think about changing this to:
| "typescript": "~4.8.4 || ^5.0.0" | |
| "typescript": "~4.8.4 || ~5.0.0" |
There was a problem hiding this comment.
Let's at least support 5.1. Changing to ~4.8.4 || ~5.0 || ~5.1.
packages/base/package.json
Outdated
| "eslint-plugin-import": "^2.26.0", | ||
| "eslint-plugin-jsdoc": "^39.6.2", | ||
| "eslint-plugin-import": "^2.27.5", | ||
| "eslint-plugin-jsdoc": "^39.6.2 || ^41 || ^43.0.7", |
There was a problem hiding this comment.
The versions referenced in each README should be updated as well
There was a problem hiding this comment.
What do we recommend that people install for new projects? Should it be ^43.0.7?
There was a problem hiding this comment.
Good question. I guess that could stay the same for now, though the eslint-plugin-import reference should be updated at least, since the minimum has been increased
There was a problem hiding this comment.
READMEs updated. Changed recommendation to ^41.1.2.
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: typescript@4.8.4 |
|
The version ranges are getting a bit complicated — I hope we can simplify them in the future — but I can live with them. I can approve this once the conflicts are resolved. |
|
@mcmire rebased |
devDependencytypescript accordingly.