Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

RebeccaStevens
Copy link
Collaborator

@RebeccaStevens RebeccaStevens commented Feb 13, 2023

PR Checklist

Overview

  • Set minimum supported version of TS to v5
  • Removes things that have become deprecated in v5
  • Uses new bundler strategy for moduleResolution
    • Removes use of .js extensions and /index paths as the bundler resolution now handles that (linting rules updated accordingly).

"vitest": "^0.28.0"
},
"peerDependencies": {
"typescript": ">=4"
"typescript": ">=5 || 5.0.0-beta"
Copy link
Owner

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Owner

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.

Copy link
Collaborator

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.

@JoshuaKGoldberg JoshuaKGoldberg added the status: in discussion Not yet ready for implementation or a pull request label Feb 14, 2023
This was referenced Feb 17, 2023
@RebeccaStevens RebeccaStevens marked this pull request as draft February 25, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not yet ready for implementation or a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants