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

fix: support typescript@5.3 #1874

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Conversation

falsepopsky
Copy link
Contributor

@falsepopsky falsepopsky commented Nov 20, 2023

Tested locally in the project with TypeScript 5.3 without errors:

  1. Run pnpm add typescript@5.3
  2. Run pnpm build
  3. Run pnpm test:ts

P.S.: If possible, can we update the pnpm version in the project? I received numerous warnings with retries, and as a result, I manually updated it to version 7.33.6 locally.

@@ -49,7 +49,7 @@ jobs:
strategy:
fail-fast: false
matrix:
ts: ['4.7', '4.8', '4.9', '5.0', '5.1', '5.2']
ts: ['4.7', '4.8', '4.9', '5.0', '5.1', '5.2', '5.3']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anybody know if TypeScript versions have EOL by design?

I really wish we dropped the left-most version when adding the latest version support. But on its own that's a breaking change, and I hate to spawn countless major versions of MSW (the TS team moves really fast). If they have the concept of EOL, perhaps that can help us reason that dropping dead versions isn't technically a breaking change since nobody should be using those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't implement semver in that way and don't keep fixing older versions generally

I think we're probably ok to be additive or even just set 4.7 as min and keep moving.

If we start shipping a type that doesn't work with 4.7 we can bump that and consider it a breaking change, but generally version to version breakage from typescript should be quite limited I would think?

Fwiw, I don't know of many libraries that test against that many versions or enforce it as exactly, so we could consider

Test minimum supposed
Test Max version
Assume everything between works
Save time and money on ci
If minimum breaks, bump the minimum and consider that a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically every release has a change that might break something, whether it's types consumer (lib dom, es..., etc) or some subtle compilation bug, so every version is somewhat breaking, but mostly those are consumer fixable, so we could probably just set a minimum occasionally too and deal with something that doesn't work in the future when it doesn't

It's much less common to have real incompatible changes, than things just working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there are some projects that follow a support window, with the most important one being DefinitelyTyped, and another one could be typescript-eslint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but generally version to version breakage from typescript should be quite limited I would think?

The practice has proved this not to be the case. I found issues between 4.5 and 4.6, I believe, and TS can and often does ship breaking changes between minor versions (you're right, they aren't semver). Because of that, we shouldn't be assuming everything from X to Y automatically works.

but mostly those are consumer fixable

Not always. MSW can declare types in a way that works on one version of TS while failing to compile the user's app on the other. It's an MSW bug, not the user's.

It's sad if there are no official dead versions of TS though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, as a consumer, id probably prefer to be able to npm install and see failures than have npm install fail, so I can start making updates to other code, which our current style of over enforcing versions for typescript doesn't make ergonomic

I'd suggest opening up the peers, and handling errors when they happen vs assuming they happen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a good decision document to me.

  1. why we are opening the peer dependencies.
  2. how to address ts-related issues (by adding the typing test target for the problematic ts version; versions without explicit tests are considered working).

this is more of a development style decision. if we allow anything and approach this on a failure-based basis, we are opening ourselves to the flow of ts issues (agree, potential) that people will be demanding to resolve. vs saying "no" to any issues on officially unsupported TS versions, giving us time to add automated tests for them before promising the support. what you propose is an optimistic promise. what we have now is overly-precautious denial of promise. but if we have active contributors such as yourself, i see little problem in switching the approach to what you suggest.

Copy link
Contributor

@mattcosta7 mattcosta7 Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, should we try to land this as is to unblock consumers from upgrading typescript, and then work on this before 5.4 ships (~3 months)

@epreston
Copy link

Maybe swapping it to ^4.7.x || ^5.2.x so its not a regular chore ?

  "peerDependencies": {
    "typescript": "^4.7.x || ^5.2.x"
  },

Constraint will be satisfied by versions matching: >=4.7.0 <5.0.0-0 || >=5.2.0 <6.0.0-0.

This is not the shortest way to write this constraint but I feel it is the clearest.
It meets these goals.

  • Version 4 typescript should be modern.
  • Version 5 typescript should be modern.

Confirmed with: https://jubianchi.github.io/semver-check/#/^4.7.x%20||%20^5.2.x/6.0

@mattcosta7
Copy link
Contributor

Maybe swapping it to ^4.7.x || ^5.2.x so its not a regular chore ?

  "peerDependencies": {
    "typescript": "^4.7.x || ^5.2.x"
  },

Constraint will be satisfied by versions matching: >=4.7.0 <5.0.0-0 || >=5.2.0 <6.0.0-0.

This is not the shortest way to write this constraint but I feel it is the clearest.
It meets these goals.

  • Version 4 typescript should be modern.
  • Version 5 typescript should be modern.

Confirmed with: https://jubianchi.github.io/semver-check/#/^4.7.x%20||%20^5.2.x/6.0

Typescript doesnt follow semver so version 4.7, 4.8, 4.9, 5.0 go in that order always, a fixed number of months between each (iirc)

So we would want something more like

= 4.7. we could probably do this until something breaks then attempt to update.

We could also try to run a test suite against rc versions of typescript and prepare fixes before the next version lands.

And we can probably say latesy 4-8 versions are supported ongoing

I don't know that we need to have these tests constantly updated to prove it? Or we could update them as desired (maybe even automatically?)

@kettanaito
Copy link
Member

I highly encourage you to read this decision document: https://github.com/mswjs/msw/blob/main/decisions/typescript-versioning.md. It's useful in the context of this discussion.

We could also try to run a test suite against rc versions of typescript and prepare fixes before the next version lands.

This implies a matrix of versions that shifts as new TS releases are out. Our pending discussion is about the fact that we cannot as easily drop previous TS versions and automatically support the latest.

@mattcosta7
Copy link
Contributor

I highly encourage you to read this decision document: https://github.com/mswjs/msw/blob/main/decisions/typescript-versioning.md. It's useful in the context of this discussion.

We could also try to run a test suite against rc versions of typescript and prepare fixes before the next version lands.

This implies a matrix of versions that shifts as new TS releases are out. Our pending discussion is about the fact that we cannot as easily drop previous TS versions and automatically support the latest.

I would expect we not add more to that matrix but instead run some action when typescript published a RC and use that single run as a test case for support for an upcoming version.

That would give some time to cleanup behaviors that break and check whether they have comparability issues or not, without requiring a constant update. I think that still might just mean testing on the oldest known working version, until it fails - and drop that on a cadence

(Although ~18 months of support also sounds fine to me as a limit)

@@ -185,7 +185,7 @@
"webpack-http-server": "^0.5.0"
},
"peerDependencies": {
"typescript": ">= 4.7.x <= 5.2.x"
"typescript": ">= 4.7.x <= 5.3.x"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why there is strict enforcement of a version? if there are incompatibilities between versions, TypeScript will be more than happy tell about it on its own by failing the build. There is questionable value and a lots of inconvenience in explicit TypeScript version management, most libraries don't do that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, there are incompatible behaviors across TypeScript versions. I recall distinctly that the same code base compiled on 4.6, failed on 4.7, and compiled again on 4.8. That's the main reason the version range is strict and was locked.

With the version testing we have in place now, it's much more reliable to open up this version range as we will catch compilation errors before we merge the changes, which is always what we want.

As Matt has said, we will merge this to unblock people (once all the pending bug fixes are released first) and then address the version range unlock as the next step.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kettanaito TS 5.4 is out, and msw again is the only dependency blocking upgrade for us.
Maybe now is a good time to address this issue conclusively?..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibertoad, I'm opening #2077 to solve this for good. Thanks for helping this happen.

@kettanaito
Copy link
Member

@mattcosta7, we can also introduce our own LTS TS support rule. For example: MSW supports TypeScript versions that are 12 month old or newer. We don't support TS versions older than 12 months.

@epreston
Copy link

Sounds good. Still run your tests on the older versions to keep you informed of community issues.

Would you consider publishing a "msw-unlocked" version to NPM that does not have this blocker and a small note saying:

"we've only tested up to typescript x.x. If you have issues, rollback to that version.

I'll gut typescript from my projects before msw. Having both of them in a project is not workable. Waiting for this projects approval to upgrade typescript just to begin my own evaluations is awkward.

@kettanaito
Copy link
Member

kettanaito commented Nov 28, 2023

@epreston, no point in publishing such a package. Just wait for the next minor version of MSW where we will lift the version lock. You can also suppress the dependency check via your package manager, it's entirely bound to it.

@danawoodman
Copy link

any chance 5.3 can be support be added soon?

@kettanaito kettanaito changed the title feat: support typescript@5.3 fix: support typescript@5.3 Jan 5, 2024
@kettanaito kettanaito merged commit 1da80bb into mswjs:main Jan 5, 2024
10 checks passed
@kettanaito
Copy link
Member

Released: v2.0.12 🎉

This has been released in v2.0.12!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump "typescript": ">= 4.7.x <= 5.2.x" to "typescript": ">= 4.7.x <= 5.3.x"
6 participants