fix: head off breaking change in ts@next #88
Conversation
|
Builds are failing because I forgot to update minimum supported TS version. I'll push these changes momentarily. |
|
Also, here's another typescript PR that is addressing some of the "weirdness" with |
BREAKING CHANGE: typescript < 3.0 will no longer be supported. To continue using simplytyped with typescript < 3, you will need to use simplytyped@1.8.0.
We don't need `Diff` any longer since `Exclude` exists (and has for a while). `Diff` is just a wrapper over `Exclude` now anyways. BREAKING CHANGE: `Diff` is no longer exported from simplytyped. Can find and replace `Diff` for `Exclude` to maintain same functionality.
`unknown` is now a built-in keyword in typescript. Since we will no longer be supporting pre-3.0 versions of typescript, we can drop the `Unknown` helper. BREAKING CHANGE: `Unknown` is no longer supported. Can find and replace all instances of `Unknown` with `unknown` to maintain same behavior.
src/types/functions.ts
Outdated
| F extends (x1: infer X1, x2: infer X2, x3: infer X3, x4: infer X4, x5: infer X5, x6: infer X6) => any ? (x1: X1, x2: X2, x3: X3, x4: X4, x5: X5, x6: X6) => R : | ||
| F extends (x1: infer X1, x2: infer X2, x3: infer X3, x4: infer X4, x5: infer X5, x6: infer X6, x7: infer X7) => any ? (x1: X1, x2: X2, x3: X3, x4: X4, x5: X5, x6: X6, x7: X7) => R : | ||
| AnyFunc<R>; | ||
| F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : AnyFunc<R>; |
There was a problem hiding this comment.
Bit of PR scope-creep, but it might be better to have never as the 'else' case here.
At least, the only case I can think of where the 'else' case matters is any, and that seems to behave better with never:
// Current implementation
type OverwriteReturn1<F extends AnyFunc, R>
= F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : AnyFunc<R>;
// Implementation with never
type OverwriteReturn2<F extends AnyFunc, R>
= F extends ((...x: infer T) => unknown) ? ((...x: T) => R) : never;
// ((...x: unknown[]) => number) | AnyFunc<number>
type Test1 = OverwriteReturn1<any, number>
// (...x: unknown[]) => number
type Test2 = OverwriteReturn2<any, number>
In practice they seem to behave the same - you can still provide arbitrary arguments to a function with unknown[] args (which surprises me - I wouldn't mind if this was actually a bit stricter, really) - but the latter just has a cleaner type.
src/types/functions.ts
Outdated
| F extends (x1: infer X1, x2: infer X2, x3: infer X3, x4: infer X4, x5: infer X5, x6: infer X6) => any ? [X1, X2, X3, X4, X5, X6] : | ||
| F extends (x1: infer X1, x2: infer X2, x3: infer X3, x4: infer X4, x5: infer X5, x6: infer X6, x7: infer X7) => any ? [X1, X2, X3, X4, X5, X6, X7] : | ||
| any[]; | ||
| F extends ((...x: infer T) => unknown) ? T : any[]; |
There was a problem hiding this comment.
Essentially same comment as OverwriteReturn:
ArgsAsTuple<any> is currently any[] | unknown[], would be unknown[] if the any[] was switched to never.
There was a problem hiding this comment.
Good point, I'll switch both of these over to never. Now that we don't really have a base case, never makes more sense than trying to be overly permissive as the old version was doing.
src/types/objects.ts
Outdated
| @@ -175,7 +174,7 @@ export type TaggedObject<T extends Record<keyof any, object>, Key extends keyof | |||
| */ | |||
| export type DeepPartial<T> = Partial<{ | |||
| [k in Keys<T>]: | |||
| T[k] extends any[] ? Array<DeepPartial<T[k][number]>> : | |||
| T[k] extends Array<unknown> ? Array<DeepPartial<T[k][number]>> : | |||
There was a problem hiding this comment.
Nit: unknown[] would be more stylistically consistent. You might need to bump tslint if it's complaining about it. (I almost managed to fix this in tslint myself: palantir/tslint#4219)
There was a problem hiding this comment.
Ah nice, yeah I also would prefer unknown[], but couldn't get tslint to be happy. I'll take a look at bumping the version.
src/impl/objects.ts
Outdated
| @@ -6,7 +6,7 @@ import { DeepReadonly, Keys, TaggedObject, ObjectKeys } from '../types/objects'; | |||
| * @param obj object to query for key `k` | |||
| * @param k key to check existence in `obj` | |||
| */ | |||
| export function isKeyOf<T extends object>(obj: T, k: keyof any): k is keyof T { | |||
| export function isKeyOf<T extends object>(obj: T, k: ObjectKeys): k is keyof T { | |||
There was a problem hiding this comment.
I think I'd actually prefer keyof any, but that's part of a larger discussion that I've been meaning to open for awhile. I'll go ahead an open an issue discussing that, rather than create a big tangent here.
BREAKING CHANGE: No longer exporting Keys<T> either. This alias is too opinionated stylistically and doesn't support good TS practices. If using it, it would be best to include in a per-project basis. Besides, it isn't difficult to write or come up with (`type Keys<T> = keyof T;`), so is outside the scope of this project.
Internal aliasing
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Turns out
extends anyis not the recommended way to do distributed conditionals. The test case forStrictUnionstarted breaking ontypescript@nextthis morning.I believe the PR where this started is here and an issue has already been filed in typescript here.
We'll need to up our minimum typescript version for this change so that we can make use of the
unknowntype. I'd like to go through and do some cleaning up of the repository anyways, so maybe I'll extend this PR to do a major version bump of simplytyped while I'm at it. There are a few type definitions in ST that can go away now with later versions of ts.The broken build is here and fails with this error:
using
typescript@3.4.0-dev.20190310.