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

feat: support TypeScript 5.2 #1711

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Conversation

chentsulin
Copy link
Contributor

Loosen peer dependency constraint to unblock projects that use msw from upgrading to TypeScript 5.2.x.

@kettanaito
Copy link
Member

I've updated the branch against the latest main.

@kettanaito kettanaito changed the title chore: add support for TypeScript 5.2.x feat: support TypeScript 5.2 Aug 25, 2023
@kettanaito
Copy link
Member

Looks like we are getting a consistent TS compilation error:

Error: src/utils/request/MockedRequest.ts(119,34): error TS2345: Argument of type 'HeadersPolyfill' is not assignable to parameter of type 'Headers'.
  Property 'getSetCookie' is missing in type 'HeadersPolyfill' but required in type 'Headers'.

I suspect that something might have changed regarding the validation in this regard, or the Headers type definitions have changed (do those ship alongside TS versions? It should come from the dom library).

@mattcosta7
Copy link
Contributor

mattcosta7 commented Aug 25, 2023

(do those ship alongside TS versions? It should come from the dom library).

lib.d.ts changes do ship with TS versions, usually noted in the breaking changes section on the release announcement - https://devblogs.microsoft.com/typescript/announcing-typescript-5-2/#breaking-changes-and-correctness-fixes

DOM type updates from 5.2 are here

looks like https://github.com/microsoft/TypeScript/pull/54725/files#diff-dc0eab937d15e62545da3ed7b4f40ad6b24f15dd88fbc6ceda2bfb4ed8356eb0R13429 is the line that added the method in question

related mdn docs - https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie

@mattcosta7
Copy link
Contributor

@kettanaito - mswjs/headers-polyfill#57 should implement the method this PR fails type checks from, on the header-polyfill package

@kettanaito
Copy link
Member

Thank you for opening a pull request in such a timely manner, @mattcosta7 🙌

My main concern is what would happen with the older versions of TypeScript where getSetCookie is not present on Headers in the lib.dom? I do expect its presence to cause an error. Since we are maintaining a rather wide range of TypeScript versions, we have to take that into account as we don't want to break the types compilation for our users on older TypeScript versions.

Could someone please give it a try on TS < 5.2? I expect it either to break or TS to be smart about it and treat a class with extra methods as satisfying the class with narrower expected methods (I hope very much that's the case).

@kettanaito
Copy link
Member

A new version of headers-polyfill has been published so this pull request can be updated with it and have the typings tests pass.

@chentsulin
Copy link
Contributor Author

Run pnpm tsc --version
Version 5.2.2

> msw@1.2.5 test:ts /Users/runner/work/msw/msw
> ts-node test/typings/run.ts

Using tsconfig at "/Users/runner/work/msw/msw/test/typings/tsconfig.5.2.json"
error TS51[10](https://github.com/mswjs/msw/actions/runs/5995405284/job/16258365832?pr=1711#step:8:11): Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
 ELIFECYCLE  Command failed with exit code 2.
Error: Process completed with exit code 1.

@mattcosta7
Copy link
Contributor

Run pnpm tsc --version
Version 5.2.2

> msw@1.2.5 test:ts /Users/runner/work/msw/msw
> ts-node test/typings/run.ts

Using tsconfig at "/Users/runner/work/msw/msw/test/typings/tsconfig.5.2.json"
error TS51[10](https://github.com/mswjs/msw/actions/runs/5995405284/job/16258365832?pr=1711#step:8:11): Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.
 ELIFECYCLE  Command failed with exit code 2.
Error: Process completed with exit code 1.

may need to change the tsconfig.5.2.2 file to use a different module - by default I think it uses commonjs which isn't compat in 5.2.2 with moduleResolution node 16 I guess

@kettanaito
Copy link
Member

@mattcosta7, that's odd. I thought that module resolution has changed as far back as 4.9/5.0. But agree on the strategy, we need to introduce a new base config for this version of TS with the right module resolution set.

@mattcosta7
Copy link
Contributor

mattcosta7 commented Aug 28, 2023

@mattcosta7, that's odd. I thought that module resolution has changed as far back as 4.9/5.0. But agree on the strategy, we need to introduce a new base config for this version of TS with the right module resolution set.

typescript added this error in 5.2 - https://devblogs.microsoft.com/typescript/announcing-typescript-5-2/#module-and-moduleresolution-must-match-under-recent-node-js-settings

so it might not have correct before, but only causes issues in 5.2+

See the PR that added this here: microsoft/TypeScript#54567

@kettanaito
Copy link
Member

kettanaito commented Aug 29, 2023

Thanks for sharing, @mattcosta7.

I've noticed 5.2 extended 4.7 config that had moduleResolution: Node16 set already. It appears that because of that moduleResolution was supposed to be set to Node16 as well.

I set explicit module and moduleResolution values to NodeNext and pushed the changes. Let me know if that's alright or we should keep Node16 for 5.2 as well. I understand it'd he nice to type test MSW with various module/moduleResolution combinations but we don't have the infrastructure for that yet. I'd recommend we go with the most common settings people will have.

@kettanaito
Copy link
Member

I've also noticed that tsup is rather old and requires old TS versions but upgrading it caused test failures in response patching with GraphQL (Jest's error about ESM support). Looks like a bigger endeavor, we don't have to address it now.

@kettanaito
Copy link
Member

The tests are passing. Let me know if any more changes are intended here. Otherwise, we can merge and release.

@chentsulin
Copy link
Contributor Author

I think we're ready to merge and release this.

@kettanaito kettanaito merged commit 2ca791e into mswjs:main Sep 1, 2023
10 checks passed
@kettanaito
Copy link
Member

Thanks for seeing this improvement through, @chentsulin, @mattcosta7!

@chentsulin chentsulin deleted the chore/support-ts-5.2 branch September 1, 2023 10:09
@azangru
Copy link

azangru commented Sep 3, 2023

Could you please release this change to npm?

@thomasburguiere
Copy link

Hello, thanks a lot for the changes, any ETA on the npm release ? 🙂

@kettanaito
Copy link
Member

Released: v1.3.0 🎉

This has been released in v1.3.0!

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.

@kettanaito kettanaito mentioned this pull request Sep 7, 2023
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants