From 9c9f0232d36891f5632f5bfea6267a4c55549fff Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Fri, 1 Apr 2022 10:25:51 -0600 Subject: [PATCH 1/2] Add failing test for distributive signature types --- test/types/component-test.ts | 70 +++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/test/types/component-test.ts b/test/types/component-test.ts index a4e32770e..45a31630c 100644 --- a/test/types/component-test.ts +++ b/test/types/component-test.ts @@ -1,5 +1,6 @@ import { expectTypeOf } from 'expect-type'; import * as gc from '@glimmer/component'; +import Component from '@glimmer/component'; // Imported from non-public-API so we can check that we are publishing what we // expect to be -- and this keeps us honest about the fact that if we *change* @@ -13,31 +14,58 @@ import * as gc from '@glimmer/component'; // which they should not use in any way, this is "safe" from a public API POV. import { EmptyObject } from '@glimmer/component/dist/types/addon/-private/component'; -const Component = gc.default; +declare let basicComponent: Component; +expectTypeOf(basicComponent).toHaveProperty('args'); +expectTypeOf(basicComponent).toHaveProperty('isDestroying'); +expectTypeOf(basicComponent).toHaveProperty('isDestroyed'); +expectTypeOf(basicComponent).toHaveProperty('willDestroy'); +expectTypeOf(basicComponent.isDestroying).toEqualTypeOf(); +expectTypeOf(basicComponent.isDestroyed).toEqualTypeOf(); +expectTypeOf(basicComponent.willDestroy).toEqualTypeOf<() => void>(); expectTypeOf(gc).toHaveProperty('default'); expectTypeOf(gc.default).toEqualTypeOf(); -type Args = { +type LegacyArgs = { foo: number; }; -const componentWithLegacyArgs = new Component({}, { foo: 123 }); -expectTypeOf(componentWithLegacyArgs).toHaveProperty('args'); -expectTypeOf(componentWithLegacyArgs).toHaveProperty('isDestroying'); -expectTypeOf(componentWithLegacyArgs).toHaveProperty('isDestroyed'); -expectTypeOf(componentWithLegacyArgs).toHaveProperty('willDestroy'); -expectTypeOf(componentWithLegacyArgs.args).toEqualTypeOf>(); -expectTypeOf(componentWithLegacyArgs.isDestroying).toEqualTypeOf(); -expectTypeOf(componentWithLegacyArgs.isDestroyed).toEqualTypeOf(); -expectTypeOf(componentWithLegacyArgs.willDestroy).toEqualTypeOf<() => void>(); +const componentWithLegacyArgs = new Component({}, { foo: 123 }); +expectTypeOf(componentWithLegacyArgs.args).toEqualTypeOf>(); + +// Here, we are testing that the types propertly distribute over union types, +// generics which extend other types, etc. +type LegacyArgsDistributive = { foo: number } | { bar: string; baz: boolean }; + +const legacyArgsDistributiveA = new Component({}, { foo: 123 }); +expectTypeOf(legacyArgsDistributiveA.args).toEqualTypeOf>(); +const legacyArgsDistributiveB = new Component({}, { bar: "hello", baz: true }); +expectTypeOf(legacyArgsDistributiveB.args).toEqualTypeOf>(); + +interface ExtensibleLegacy { + value: T; + extras: boolean; + funThings: string[]; +} + +class WithExtensibleLegacy> extends Component {} +declare const withExtensibleLegacy: WithExtensibleLegacy>; +expectTypeOf(withExtensibleLegacy.args.value).toEqualTypeOf(); +expectTypeOf(withExtensibleLegacy.args.extras).toEqualTypeOf(); +expectTypeOf(withExtensibleLegacy.args.funThings).toEqualTypeOf(); + +interface Extended extends ExtensibleLegacy {} + +class WithExtensibleLegacySubclass extends WithExtensibleLegacy {} +declare const withExtensibleLegacySubclass: WithExtensibleLegacySubclass; +expectTypeOf(withExtensibleLegacySubclass.args.value).toEqualTypeOf(); interface ArgsOnly { - Args: Args; + Args: LegacyArgs; } const componentWithArgsOnly = new Component({}, { foo: 123 }); -expectTypeOf(componentWithArgsOnly.args).toEqualTypeOf>(); +expectTypeOf(componentWithArgsOnly.args).toEqualTypeOf>(); interface ElementOnly { Element: HTMLParagraphElement; @@ -61,33 +89,33 @@ const componentWithBlockOnly = new Component({}, {}); expectTypeOf(componentWithBlockOnly.args).toEqualTypeOf>(); interface ArgsAndBlocks { - Args: Args; + Args: LegacyArgs; Blocks: Blocks; } const componentwithArgsAndBlocks = new Component({}, { foo: 123 }); -expectTypeOf(componentwithArgsAndBlocks.args).toEqualTypeOf>(); +expectTypeOf(componentwithArgsAndBlocks.args).toEqualTypeOf>(); interface ArgsAndEl { - Args: Args; + Args: LegacyArgs; Element: HTMLParagraphElement; } const componentwithArgsAndEl = new Component({}, { foo: 123 }); -expectTypeOf(componentwithArgsAndEl.args).toEqualTypeOf>(); +expectTypeOf(componentwithArgsAndEl.args).toEqualTypeOf>(); interface FullShortSig { - Args: Args; + Args: LegacyArgs; Element: HTMLParagraphElement; Blocks: Blocks; } const componentWithFullShortSig = new Component({}, { foo: 123 }); -expectTypeOf(componentWithFullShortSig.args).toEqualTypeOf>(); +expectTypeOf(componentWithFullShortSig.args).toEqualTypeOf>(); interface FullLongSig { Args: { - Named: Args; + Named: LegacyArgs; Positional: []; }; Element: HTMLParagraphElement; @@ -101,4 +129,4 @@ interface FullLongSig { } const componentWithFullSig = new Component({}, { foo: 123 }); -expectTypeOf(componentWithFullSig.args).toEqualTypeOf>(); +expectTypeOf(componentWithFullSig.args).toEqualTypeOf>(); From ad429d570e997f6184e4948f69a24f927bdd122b Mon Sep 17 00:00:00 2001 From: Chris Krycho Date: Fri, 1 Apr 2022 10:53:50 -0600 Subject: [PATCH 2/2] Fix type distributivity in Glimmer Component Signature When using a `keyof` type to check whether the type parameter for Glimmer Component is a `Signature` or the classic `Args`-only type, if we do not force TS to distribute over union types, it resolves the `keyof` check for union types with no shared members as `never`, and `never extends ` is always true. This in turn meant that for all such unions, as well as for cases where users were providing generic types which could then be further extended in their own subclasses. Accordingly, introduce the standard technique TypeScript provides for opting into distributivity: conditional types are documented to support exactly this. --- .../component/addon/-private/component.ts | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/packages/@glimmer/component/addon/-private/component.ts b/packages/@glimmer/component/addon/-private/component.ts index 154d2edd2..d92fd1686 100644 --- a/packages/@glimmer/component/addon/-private/component.ts +++ b/packages/@glimmer/component/addon/-private/component.ts @@ -54,18 +54,7 @@ type ArgsFor = S extends { Args: infer Args } : { Named: S['Args']; Positional: [] } : { Named: EmptyObject; Positional: [] }; -/** - * Given any allowed shorthand form of a signature, desugars it to its full - * expanded type. - * - * @internal This is only exported so we can avoid duplicating it in - * [Glint](https://github.com/typed-ember/glint) or other such tooling. It is - * *not* intended for public usage, and the specific mechanics it uses may - * change at any time. Although the signature produced by is part of Glimmer's - * public API the existence and mechanics of this specific symbol are *not*, - * so ***DO NOT RELY ON IT***. - */ -export type ExpandSignature = { +type _ExpandSignature = { Element: GetOrElse; Args: keyof T extends 'Args' | 'Element' | 'Blocks' // Is this a `Signature`? ? ArgsFor // Then use `Signature` args @@ -74,11 +63,29 @@ export type ExpandSignature = { ? { [Block in keyof Blocks]: Blocks[Block] extends unknown[] ? { Positional: Blocks[Block] } - : Blocks[Block]; + : Blocks[Block] } : EmptyObject; }; +/** + * Given any allowed shorthand form of a signature, desugars it to its full + * expanded type. + * + * @internal This is only exported so we can avoid duplicating it in + * [Glint](https://github.com/typed-ember/glint) or other such tooling. It is + * *not* intended for public usage, and the specific mechanics it uses may + * change at any time. Although the signature produced by is part of Glimmer's + * public API the existence and mechanics of this specific symbol are *not*, + * so ***DO NOT RELY ON IT***. + */ +// The conditional type here is because TS applies conditional types +// distributively. This means that for union types, checks like `keyof T` get +// all the keys from all elements of the union, instead of ending up as `never` +// and then always falling into the `Signature` path instead of falling back to +// the legacy args handling path. +export type ExpandSignature = T extends any ? _ExpandSignature : never; + /** * @internal we use this type for convenience internally; inference means users * should not normally need to name it @@ -220,7 +227,9 @@ export default class BaseComponent { constructor(owner: unknown, args: Args) { if (DEBUG && !(owner !== null && typeof owner === 'object' && ARGS_SET.has(args))) { throw new Error( - `You must pass both the owner and args to super() in your component: ${this.constructor.name}. You can pass them directly, or use ...arguments to pass all arguments through.` + `You must pass both the owner and args to super() in your component: ${ + this.constructor.name + }. You can pass them directly, or use ...arguments to pass all arguments through.` ); }