-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
typescript: add support for typescript 5.0.x, 5.1.x #288
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
@@ -35,7 +35,7 @@ | |||
"eslint": "^8.27.0", | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}, | ||
"peerDependencies": { | ||
"@metamask/eslint-config": "^11.0.0", | ||
"@typescript-eslint/eslint-plugin": "^5.42.1", | ||
"@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The versions referenced in each README should be updated as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we recommend that people install for new projects? Should it be ^43.0.7
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
devDependency
typescript accordingly.