Upgrade to TypeScript 4.4

[benmccann]

Describe the problem

We're on TypeScript 4.3

Describe the proposed solution

Upgrade to 4.4

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

[Theo-Steiner]

@benmccann you may already know this, but for now rollup builds get stuck with typescript 4.4 or greater, making this milestone dependant on this upstream issue: rollup/plugins#983

[benmccann]

Thanks. I wasn't aware, but expect we won't face that issue because we don't use @rollup/plugin-typescript

[Conduitry]

Vite does. I don't know whether it's just part of its own build process or whether the shipped package ends up containing a bundled copy of it.

[benmccann]

I still imagine it shouldn't matter for us. We don't ship the TypeScript library. It will just enable slightly stricter type checking on our own code when we run pnpm check. And I don't think Vite would run @rollup/plugin-typescript on our code because we haven't added that to our plugin config

[vhfmag]

I've just tried to work on this issue and I think I got it to work, but I'm not sure what path to take around handling a breaking change:

After upgrading, all the new TypeScript compilation errors were caused by the new useUnknownInCatchVariables flag, automatically enabled in the project because of the strict flag. To work around the issue, there are a couple paths:

1. Disable the useUnknownInCatchVariables flag in tsconfig.json
2. Explicitly type all catch bindings (e.g. catch (/** @type {any} */ error))
3. Type check the catch binding in runtime

My solution so far took path 3, creating a utility assertError function that throws if its argument isn't instanceof Error, so that after calling it, the variable's type is safely narrowed to Error. I thought I'd ask if it's desirable or if I should change course before opening a PR.

---

Edit: Removed catch (/** @type {Error} */ error) example since catch bindings can only be typed as any or unknown

[benmccann]

That sounds reasonable to me. @ignatiusmb @dummdidumm are more of TypeScript experts than I am though

[benmccann]

Btw, we also have a coalesce_to_error function that's used some places. We should probably remove the typecast to unknown before that function is called when we upgrade to 4.4

[dummdidumm]

Sounds reasonable to me, but we need to be careful that these are all instanceof Error. For example getRawBody of node/index.js rejects and I'm not 100% sure if the rejected value is a instance/subclass of Error (but I would hope so).

[bluwy]

I think it's also worth noting that Error might not actually be the nodejs error we want, but the browser error instead, which lacks some extra properties. More info. I also encountered this issue for vite-plugin-svelte and it's really tangled for what seem to be simple. I'd be interested to see how the checks are implemented here as well.

Explicitly type all catch bindings (e.g. catch (/** @type {any} */ error) or catch (/** @type {Error} */ error))

I tried this as well but in a typescript file, and typescript actually explicitly prevents this pattern (throws error), e.g. catch (error: any). It could be a different story for jsdoc.

NOTE: vite-plugin-svelte disables useUnknownInCatchVariables at the moment.

[vhfmag]

Explicitly type all catch bindings (e.g. catch (/** @type {any} */ error) or catch (/** @type {Error} */ error))

I tried this as well but in a typescript file, and typescript actually explicitly prevents this pattern (throws error), e.g. catch (error: any). It could be a different story for jsdoc.

You're correct, catch bindings can only be typed as any or unknown, even in JSDoc. I'll edit my original comment to clarify that.

---

Sounds reasonable to me, but we need to be careful that these are all instanceof Error. For example getRawBody of node/index.js rejects and I'm not 100% sure if the rejected value is a instance/subclass of Error (but I would hope so).

I worried non-Error values could be thrown too, but relaxed a little after all tests ran without triggering the assertion errors. Now, investigating further, I found one instance of throwing/rejecting a non-Error value and two catch bindings expecting its shape (1, 2). Maybe I should create a HttpError class with status and reason fields to be thrown in place of the plain object, and type check it with assertInstanceOf(error, HttpError)?

Otherwise, is there some additional way to test if it's safe to implement the checks as instanceof Error?

---

I think it's also worth noting that Error might not actually be the nodejs error we want, but the browser error instead, which lacks some extra properties. More info.

As is recommended by the linked article, I made sure assertError makes error type to be inferred as Error & NodeJS.ErrnoException. Since all the extra properties are optional, I don't think additional checks are needed. How does it sound?

[dummdidumm]

I worried non-Error values could be thrown too, but relaxed a little after all tests ran without triggering the assertion errors. Now, investigating further, I found one instance of throwing/rejecting a non-Error value and two catch bindings expecting its shape (1, 2). Maybe I should create a HttpError class with status and reason fields to be thrown in place of the plain object, and type check it with assertInstanceOf(error, HttpError)?

I think the safer solution would be create such a HttpError as a sublcass of Error and leave the check as is (instanceof Error) so unforseen Errors during other code paths will still pass the assertion.

In general I'm a little hesitant to use option 3 just because such an error could by anything and we can only make assumptions about its structure given the code paths we see.

Otherwise, is there some additional way to test if it's safe to implement the checks as instanceof Error?

I don't think there's a good way to test this, the best way I can think of is to check the code paths that lead to these errors.

[bluwy]

As is recommended by the linked article, I made sure assertError makes error type to be inferred as Error & NodeJS.ErrnoException. Since all the extra properties are optional, I don't think additional checks are needed. How does it sound?

Yeah sounds good if NodeJS.ErrnoException is used for the typeguard too.

Unrelated: If you do make a PR, make sure internal functions are snake_case (https://github.com/sveltejs/kit#coding-style)