Move to TypeScript

[mgcrea]

Following #321, I took some time today to attempt a port to TypeScript with minimal changes.

Work is not fully done yet (did not update the CI pipeline, hence the failed tests), but it does pass typescript validation.

There is a two places where I had to @ts-expect-error, related to dynamically assigned props as I'm not sure yet how I can port it to Typescript (could be done easily by assigning each props without the loop, see reviews). Open to ideas on this.

Also there is some remaining typing issues (existing in the current code but invisible) regarding the actual options passed around (see InternalOptions vs NormalizedOptions) so I had to add a few manual as casting as well that should be fixed (but could be done after the migration to keep changes to a minimum).

Also I had to refactor the Ky class API a bit (added a static Ky.create) that did return a ResponsePromise from its constructor instead of the expected Ky instance as it looks like TypeScript forbids class constructor return type overloads.

I tried my best to follow what was made for got and to not change anything in the code yet (I have quite a few changes that I'd like to propose next but that were out of scope).

Before I spend too much more time on this I'd love to get some feedback on the direction of this merge request and the probability of it being merged once ready.

I'm willing to change anything that you would not like (naming, etc.).

Thanks for the review!

[sholladay]

Hi @mgcrea and thanks for your effort. I'll do a proper review once Sindre chimes in with whether he wants to move forward with this (given the comments on #321) and the tests are passing. I don't have strong feelings myself, except to say that - as you've encountered - some of the patterns we use in Ky don't seem to be fully supported by TypeScript and I dislike writing code differently than we would otherwise just to please the compiler. Especially when it comes to the public API, which seems to be the case here with the static Ky.create if I understand correctly.

That said, for Deno users like myself this would be awesome. The current approach to loading type definition files in Deno is kludgy and non-standard, so I'd love to remove the need for that.

[mgcrea]

@sholladay thanks! If I'm not mistaken, new Ky() was an internal API so I don't think there is any major issue here. I understand your point about having to please the compiler, but in most cases I find the advices pretty sound and that do lead to code both simpler understand and debug.

Waiting for @sindresorhus blessings to continue with unit tests.

[sholladay]

Correct, the Ky class is an internal concept. I thought that making create() a static method implied breaking the ability to call create() on an instance, but I can see in the code that you've kept that ability, so never mind on that.

[sindresorhus]

I'm ok with moving forward with this as the code is complicated and it's nice having some type safety when refactoring or adding features.

[sindresorhus]

What's the plan for testing?

We cannot move to ESM because AVA (test runner) doesn't yet support ESM with TS. - #321 (comment)

[mgcrea]

@sindresorhus Haven't really thought about it yet but I guess we could use a specific tsconfig build for tests (without ESM) along with @ava/typescript, and still output multiple independent builds depending on what we want to support (amd/cjs/esm)?

I'm usually using ts-jest in my projects where ts is a first-class citizen. So I could also easily port the testing setup to use jest but I guess you probably want to keep testing uniform among your projects and stick with ava.

Will try to have something working pretty soon.

[sindresorhus]

we could use a specific tsconfig build for tests (without ESM) along with @ava/typescript

I guess that might work.

and still output multiple independent builds depending on what we want to support (amd/cjs/esm)?

ESM is the only production target.

[mgcrea]

@sindresorhus I've just finished to fully migrate the tests to typescript. Was a bit painful to find the exact working setup to properly test the ESM build from non-ESM compiled unit-tests that nyc/ava can digest, but ended up finding something thanks to playwright debug mode.

I did bump all dev deps, so I had to fix a few linting-related issues in the code to have xo-related test pass, disabled a couple of eslint rules as well either related to typescript and to keep changes to a minimum.

Had to rewrite a bit the helpers part related to createHttpServer that was using an old non-ts module (took the helper directly from got) and had to add a couple of helpers to properly load the ESM build from chrome.

I went with named exports across the source code coupled with all-star-exports index.js files which is a setup I heavily use nowadays, however for an ESM-published lib, we might want to keep files to a minimum and explicitly import every dep. Can refactor that quickly if you want (or can be done later).

Also had to add .js extensions everywhere so that the esm build properly works (since typescript don't want to have anything with import-related transforms).

If you find any issue blocking the merge don't hesitate to ping me and I'll do my best to address it / fix it in a timely manner.

Another thing we should decide before publishing a release is the tsc compiler target, currently es2019 I guess would not work in some old browsers.

[sindresorhus]

Adhere to the linting.

[mgcrea]

Unfortunately typescript won't let me use a for-of loop in that case, getting Type 'Headers' must have a '[Symbol.iterator]()' method that returns an iterator. and no entries() method declared.

edit: fixed by adding missing "DOM.Iterable" to tsconfig.json lib.

[sindresorhus]

Don't disable this rule.

[mgcrea]

Done for quotes, have to keep dot-notation off as typescript forbids dot notation in quite a few header-related cases (due to specific type) and disabling by line would be tedious/verbose.

[sindresorhus]

Commented on some nitpick. Will do a more in-depth review tomorrow.

[sindresorhus]

Another thing we should decide before publishing a release is the tsc compiler target, currently es2019 I guess would not work in some old browsers.

es2019 is fine. It's what it currently is (Node.js 12). Ky only targets modern browsers.

[mgcrea]

Looking at the code today I realised that during my tests I had dropped the "type": "module" from the package.json, unfortunately adding it back does break testing (since test are built with require but require is forbidden when type is module). So I had to hack something around by monkey-patching a copied package.json with commonjs module inside the dist folder.

Hopefully nyc/ava adds support for ESM soon so we can simplify the build/test.

[sindresorhus]

Regarding testing, seems there's a solution with ts-node: https://github.com/avajs/ava/blob/main/docs/recipes/typescript.md#for-packages-with-type-module

[mgcrea]

Just finished implementing the solution you linked and it drastically simplified the testing process as it no longer requires building the tests (so no more dist/ folder) and no package.json monkey-patching needed!

One downside however is that we can no longer test against NodeJS v12, but since it's a browser lib I'm not sure there is much benefit testing against a matrix of NodeJS versions (ideally we should test against a matrix of browsers).

[sholladay]

One downside however is that we can no longer test against NodeJS v12

We do support Node when combined with polyfills, but I'm fine with dropping Node 12 from the test matrix.

ideally we should test against a matrix of browsers

That's what test/browser.js does. It uses Playwright to run some tests in a headless browser. It's currently set up to test against Chromium, but we could configure it to test Webkit and Firefox as well.

[sindresorhus]

but I'm fine with dropping Node 12 from the test matrix.

👍

[mgcrea]

@sindresorhus I've dropped the node@12 from the GitHub action, is there anything else I can do to move this pr forward?

[sindresorhus]

@mgcrea Don't force push. It makes it impossible to review only what changed.

[sindresorhus]

@mgcrea Did you see @sholladay's feedback?

[mgcrea]

Yes and I think I've answered every comment, to me there is nothing really blocking left (but I could be wrong). Point taken regarding force-pushing, won't do it again.

[sholladay]

I don't see those responses to my comments, @mgcrea.

[mgcrea]

Unfortunately typescript won't let me use a for-of loop in that case, getting Type 'Headers' must have a '[Symbol.iterator]()' method that returns an iterator. and no entries() method declared.

edit: fixed by adding missing "DOM.Iterable" to tsconfig.json lib.

[mgcrea]

Indeed it looks like I did not properly understand how the reviewing process works, all my comments were flagged as pending and I thought you could see them. I had to « submit review » to properly publish them...! 😅 Good to know. I also marked every conversation as resolved as an earlier attempt to publish so feel free to unresolve the points you want to have addressed / discussed.

[sindresorhus]

For the remaining @ts-expect-error. I suggest we just leave it and open an issue about fixing @ts-expect-error. Maybe someone else knows how to correctly type those.

We can link to https://github.com/sindresorhus/ky/search?q=%22@ts-expect-error%22

[sindresorhus]

@mgcrea Would you be able to fix the failing tests?

[mgcrea]

@sindresorhus forgot to push a commit fixing the broken test due to the updated dist path, should be ok now. Looks like the workflow is stuck (new security feature of GitHub) pending your approval.

[sindresorhus]

@mgcrea Thanks for working on this 👍

[mgcrea]

Thanks for merging such a big PR! Looking forward to contribute with smaller patches in the future!