-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
build: update to TS v5 #53
Conversation
BREAKING CHANGE: require TS 5.0 or newer
…`"moduleResolution": "bundler"`
"vitest": "^0.28.0" | ||
}, | ||
"peerDependencies": { | ||
"typescript": ">=4" | ||
"typescript": ">=5 || 5.0.0-beta" |
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 theory, I love this - easier imports, lots of removed functions, nice clean minimum TypeScript version.
In practice, microsoft/TypeScript#51362 notes that TS v5 comes out in a month, on March 14th. So folks will be in a hard position till then: either they stay on tsutils
, stay on an older version of ts-api-utils
, or use a beta version of TypeScript. What do you think about waiting till then @RebeccaStevens?
cc @bradzacher from typescript-eslint/typescript-eslint#6380 & typescript-eslint/typescript-eslint#6459, would the typescript@5 requirement be inconvenient to you?
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.
To me, waiting a month before making a 1.0 release is no big deal - it gives us plenty of time to get this library right. I guess it really just depends on when typescript-eslint want to release v6.
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.
Making it the minimum would prevent us from using it really because we support older TS versions.
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 don't actually rely on any v5 things so I guess we don't need to set the minimum version to v5.
We can just design the utility functions around v5 and then add any v4 one needed by typescript-eslint or other dependents. Or if we re-export/redefine type guards already defined in TypeScript (#51), then we can easily support older version of TypeScript.
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 context as of our upcoming v6 release (which is when we'll switch to this package) we'll be bumping the minimum TS version to 4.2 - which is the same minimum that DefinitelyTyped supports.
So we could do the same thing here and keep everything perfectly balanced, as all things should be.
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 like keeping the minimum TS version synced to DefinitelyTyped and typescript-eslint, if these can all be the same.
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 maintain safety in the codebase we use patch-package
to edit the TS types to help flag when we use things that aren't in older versions. I think it's a great strategy we could use here if needs be.
PR Checklist
status: accepting prs
Overview
bundler
strategy formoduleResolution
.js
extensions and/index
paths as thebundler
resolution now handles that (linting rules updated accordingly).