Merge pull request #391 from glimmerjs/bugfix-signature-master

chadhietala · web-flow · commit 8edbbb849fae · 2022-04-01T14:04:19.000-04:00
Fix Signature handling for unions and generics
diff --git a/packages/@glimmer/component/addon/-private/component.ts b/packages/@glimmer/component/addon/-private/component.ts
@@ -60,18 +60,7 @@ type ArgsFor<S> = 'Args' extends keyof S
     : { 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<T> = {
+type _ExpandSignature<T> = {
   Element: GetOrElse<T, 'Element', null>;
   Args: keyof T extends 'Args' | 'Element' | 'Blocks' // Is this a `Signature`?
     ? ArgsFor<T> // Then use `Signature` args
@@ -84,6 +73,24 @@ export type ExpandSignature<T> = {
       }
     : 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.
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type ExpandSignature<T> = T extends any ? _ExpandSignature<T> : never;
 
 /**
  * @internal we use this type for convenience internally; inference means users
diff --git a/test/types/component-test.ts b/test/types/component-test.ts
@@ -1,37 +1,71 @@
 import { expectTypeOf } from 'expect-type';
+
+// Intentionally checking the shape of the exports *and* the export itself.
 import * as gc from '@glimmer/component';
+// tslint:disable-next-line: no-duplicate-imports
+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*
 // this import location, we've broken any existing declarations published using
 // the current type signatures.
 import { EmptyObject } from '@glimmer/component/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<boolean>();
+expectTypeOf(basicComponent.isDestroyed).toEqualTypeOf<boolean>();
+expectTypeOf(basicComponent.willDestroy).toEqualTypeOf<() => void>();
 
 expectTypeOf(gc).toHaveProperty('default');
 expectTypeOf(gc.default).toEqualTypeOf<typeof Component>();
 
-type Args = {
+type LegacyArgs = {
   foo: number;
 };
 
-const componentWithLegacyArgs = new Component<Args>({}, { foo: 123 });
-expectTypeOf(componentWithLegacyArgs).toHaveProperty('args');
-expectTypeOf(componentWithLegacyArgs).toHaveProperty('isDestroying');
-expectTypeOf(componentWithLegacyArgs).toHaveProperty('isDestroyed');
-expectTypeOf(componentWithLegacyArgs).toHaveProperty('willDestroy');
-expectTypeOf(componentWithLegacyArgs.args).toEqualTypeOf<Readonly<Args>>();
-expectTypeOf(componentWithLegacyArgs.isDestroying).toEqualTypeOf<boolean>();
-expectTypeOf(componentWithLegacyArgs.isDestroyed).toEqualTypeOf<boolean>();
-expectTypeOf(componentWithLegacyArgs.willDestroy).toEqualTypeOf<() => void>();
+const componentWithLegacyArgs = new Component<LegacyArgs>({}, { foo: 123 });
+expectTypeOf(componentWithLegacyArgs.args).toEqualTypeOf<Readonly<LegacyArgs>>();
+
+// Here, we are testing that the types propertly distribute over union types,
+// generics which extend other types, etc.
+// 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<LegacyArgsDistributive>({}, { foo: 123 });
+expectTypeOf(legacyArgsDistributiveA.args).toEqualTypeOf<Readonly<LegacyArgsDistributive>>();
+const legacyArgsDistributiveB = new Component<LegacyArgsDistributive>(
+  {},
+  { bar: 'hello', baz: true }
+);
+expectTypeOf(legacyArgsDistributiveB.args).toEqualTypeOf<Readonly<LegacyArgsDistributive>>();
+
+interface ExtensibleLegacy<T> {
+  value: T;
+  extras: boolean;
+  funThings: string[];
+}
+
+class WithExtensibleLegacy<T extends ExtensibleLegacy<unknown>> extends Component<T> {}
+declare const withExtensibleLegacy: WithExtensibleLegacy<ExtensibleLegacy<unknown>>;
+expectTypeOf(withExtensibleLegacy.args.value).toEqualTypeOf<unknown>();
+expectTypeOf(withExtensibleLegacy.args.extras).toEqualTypeOf<boolean>();
+expectTypeOf(withExtensibleLegacy.args.funThings).toEqualTypeOf<string[]>();
+
+class WithExtensibleLegacySubclass extends WithExtensibleLegacy<ExtensibleLegacy<string>> {}
+declare const withExtensibleLegacySubclass: WithExtensibleLegacySubclass;
+expectTypeOf(withExtensibleLegacySubclass.args.value).toEqualTypeOf<string>();
 
 interface ArgsOnly {
-  Args: Args;
+  Args: LegacyArgs;
 }
 
 const componentWithArgsOnly = new Component<ArgsOnly>({}, { foo: 123 });
-expectTypeOf(componentWithArgsOnly.args).toEqualTypeOf<Readonly<Args>>();
+expectTypeOf(componentWithArgsOnly.args).toEqualTypeOf<Readonly<LegacyArgs>>();
 
 interface ElementOnly {
   Element: HTMLParagraphElement;
@@ -55,33 +89,33 @@ const componentWithBlockOnly = new Component<BlockOnlySig>({}, {});
 expectTypeOf(componentWithBlockOnly.args).toEqualTypeOf<Readonly<EmptyObject>>();
 
 interface ArgsAndBlocks {
-  Args: Args;
+  Args: LegacyArgs;
   Blocks: Blocks;
 }
 
 const componentwithArgsAndBlocks = new Component<ArgsAndBlocks>({}, { foo: 123 });
-expectTypeOf(componentwithArgsAndBlocks.args).toEqualTypeOf<Readonly<Args>>();
+expectTypeOf(componentwithArgsAndBlocks.args).toEqualTypeOf<Readonly<LegacyArgs>>();
 
 interface ArgsAndEl {
-  Args: Args;
+  Args: LegacyArgs;
   Element: HTMLParagraphElement;
 }
 
 const componentwithArgsAndEl = new Component<ArgsAndEl>({}, { foo: 123 });
-expectTypeOf(componentwithArgsAndEl.args).toEqualTypeOf<Readonly<Args>>();
+expectTypeOf(componentwithArgsAndEl.args).toEqualTypeOf<Readonly<LegacyArgs>>();
 
 interface FullShortSig {
-  Args: Args;
+  Args: LegacyArgs;
   Element: HTMLParagraphElement;
   Blocks: Blocks;
 }
 
 const componentWithFullShortSig = new Component<FullShortSig>({}, { foo: 123 });
-expectTypeOf(componentWithFullShortSig.args).toEqualTypeOf<Readonly<Args>>();
+expectTypeOf(componentWithFullShortSig.args).toEqualTypeOf<Readonly<LegacyArgs>>();
 
 interface FullLongSig {
   Args: {
-    Named: Args;
+    Named: LegacyArgs;
     Positional: [];
   };
   Element: HTMLParagraphElement;
@@ -95,4 +129,4 @@ interface FullLongSig {
 }
 
 const componentWithFullSig = new Component<FullLongSig>({}, { foo: 123 });
-expectTypeOf(componentWithFullSig.args).toEqualTypeOf<Readonly<Args>>();
+expectTypeOf(componentWithFullSig.args).toEqualTypeOf<Readonly<LegacyArgs>>();
