fix!: Fix mergeProps, createElemPropsHook, and composeHooks types (#2980

)

- Upgrade TypeScript to 5.0
- Upgrade ESLint to 5.61
- Fix `mergeProps` type signature
- Fix `createElemProps` type signature
- Fix `composeHooks` type signature

More info: #2979

[category:Components]

### BREAKING CHANGES
`elemProps` hooks using `composeHooks` have more accurate type signatures which may lead to new type errors. For information, view our [discussion](#2979).

modules/docs/mdx/12.0-UPGRADE-GUIDE.mdx

@@ -40,6 +40,8 @@ A note to the reader:
   - [Form Field Container](#form-field-container)
 - [Removals](#removals)
   - [Input Icon Container](#input-icon-container)
+- [Infrastructure](#infrastructure)
+  - [TypeScript](#typescript)
 - [Component Updates](#component-updates)
   - [Styling API and CSS Tokens](#styling-api-and-css-tokens)
   - [Avatar](#avatar)
@@ -51,6 +53,7 @@ A note to the reader:
   - [Select](#select)
   - [Text Area](#text-area)
   - [Text Input](#text-input)
+- [Utility Updates](#utility-updates)
 - [Troubleshooting](#troubleshooting)
 - [Glossary](#glossary)
   - [Main](#main)
@@ -72,7 +75,7 @@ automatically update your code to work with most of the breaking changes in v12.
 handled by the codemod are marked with 🤖 in the Upgrade Guide.**
 
 > **Note: In v12, we have done some infrastructure updates with moving to Storybook 7, Webpack 5,
-> Typescript 4.9 and Cypress 13 . With these updates has come some formatting issues after running
+> TypeScript 5.0 and Cypress 13 . With these updates has come some formatting issues after running
 > our codemods. We recommend running a formatter to address the format issues that have been
 > introduced in v12.**
 
@@ -188,6 +191,18 @@ from Main instead.
 
 ---
 
+## Infrastructure
+
+### TypeScript
+
+**PR:** [#2908](https://github.com/Workday/canvas-kit/pull/2908)
+
+We've upgraded to TypeScript 5.0 to make use of
+[const Type Parameters](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#const-type-parameters).
+You will need to upgrade to TypeScript 5.0+ to avoid any TypeScript syntax errors. TypeScript does
+not follow semver, so 5.0 doesn't mean a large breaking change from 4.9. TypeScript doesn't have a
+`x.10` release, they bump the `x.9` to `{x+1}.0`.
+
 ## Component Updates
 
 ### Styling API and CSS Tokens
@@ -569,6 +584,41 @@ const theme: PartialEmotionCanvasTheme = {
 </CanvasProvider>;
 ```
 
+## Utility Updates
+
+**PR:** [#2908](https://github.com/Workday/canvas-kit/pull/2908)
+
+### `mergeProps`
+
+`mergeProps` had a bug where sometimes the returned props would be `never`. Also `mergeProps` would
+not narrow types which would require you to add `as const`. We fixed the type signature to more
+accurately reflect how `mergeProps` works. This may catch new type errors not caught before. There
+is no way to codemod this. Let us know if you need help fixing new type errors introduced by this
+change.
+
+More information: https://github.com/Workday/canvas-kit/discussions/2979
+
+### `createElemPropsHook`
+
+`createElemPropsHook` now uses
+[const Type Parameters](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#const-type-parameters)
+to narrow types in the object. This prevents requiring `as const` in some situations. This alone
+should fix bugs instead of introduce them.
+
+More information: https://github.com/Workday/canvas-kit/discussions/2979
+
+### `composeHooks`
+
+`composeHooks` uses `mergeProps` and suffered the same bugs. If any hook in the `composeHooks` chain
+used a `null` prop, the entire prop object returned was typed as `never`. This caused a bug where if
+the Component required a prop, it wasn't being provided by the composed hook. Some of our components
+manually added to the component's prop interface so the component's render function wouldn't
+complain. This has been fixed. This may be a breaking change where before the spread `elemProps` was
+`never`, so no type conflicts could exist with component props. Now all props are properly
+represented which may mean TypeScript is now catching bugs it didn't before.
+
+More information: https://github.com/Workday/canvas-kit/discussions/2979
+
 ## Troubleshooting
 
 ### My Styles Seem Broken?

modules/docs/package.json

@@ -58,6 +58,6 @@
     "fs-extra": "^10.0.0",
     "glob": "^7.1.6",
     "mkdirp": "^1.0.3",
-    "typescript": "4.9"
+    "typescript": "5.0"
   }
 }

modules/react/collection/lib/useOverflowListTarget.tsx

@@ -8,7 +8,7 @@ import {useOverflowListModel} from './useOverflowListModel';
 const hiddenStyle = {
   position: 'absolute',
   left: -99999,
-};
+} as const;
 
 /**
  * This elemProps hook measures an overflow list target and reports it to an `OverflowListModel`.

modules/react/common/lib/utils/components.ts

@@ -1,7 +1,7 @@
 import React from 'react';
 import {assert} from './assert';
 import {memoize} from './memoize';
-import {mergeProps} from './mergeProps';
+import {MergeProps, mergeProps, RemoveNulls} from './mergeProps';
 import {Model} from './models';
 
 /**
@@ -600,7 +600,7 @@ export const createComponent =
  */
 export const createElemPropsHook =
   <TModelHook extends (config: any) => Model<any, any>>(modelHook: TModelHook) =>
-  <PO extends {}, PI extends {}>(
+  <const PO extends {}, const PI extends {}>(
     fn: (
       model: TModelHook extends (config: any) => infer TModel ? TModel : Model<any, any>,
       ref?: React.Ref<unknown>,
@@ -614,6 +614,7 @@ export const createElemPropsHook =
       const props = mergeProps(fn(model, ref, elemProps || ({} as any)), elemProps || ({} as any));
       if (!props.hasOwnProperty('ref') && ref) {
         // This is the weird "incoming ref isn't in props, but outgoing ref is in props" thing
+        // @ts-ignore TS says `ref` isn't on `PO`, but we always add it anyways
         props.ref = ref;
       }
       return props;
@@ -664,6 +665,7 @@ export const createHook = <M extends Model<any, any>, PO extends {}, PI extends
     const props = mergeProps(fn(model, ref, elemProps || ({} as any)), elemProps || ({} as any));
     if (!props.hasOwnProperty('ref') && ref) {
       // This is the weird "incoming ref isn't in props, but outgoing ref is in props" thing
+      // @ts-ignore TS says `ref` isn't on `PO`, but we always add it anyways
       props.ref = ref;
     }
     return props;
@@ -881,8 +883,12 @@ export function useModelContext<T>(
 /**
  * Compose many hooks together. Each hook should make a call to `mergeProps` which is automatically
  * done by `createElemPropsHook` and `createHook. Returns a function that will receive a model and
- * return props to be applied to a component. Hooks run from right to left, but props override from
- * left to right.
+ * return props to be applied to a component. Hooks run from last to first, but props override from
+ * first to last. This means the last hook will run first, passing `elemProps` to the next last
+ * hook. There is a special exception, which is `null`. `null` means "remove this prop" and the null
+ * handling takes precedence to the first. Take care when using `null` as it will remove props
+ * passed in even from the developer. It can be useful when passing data between composed hooks or
+ * then redirecting a prop somewhere else.
  *
  * For example:
  *
@@ -891,45 +897,52 @@ export function useModelContext<T>(
  *   console.log('useHook1', elemProps)
  *   return {
  *     a: 'useHook1',
- *     c: 'useHook1'
+ *     c: 'useHook1',
+ *     d: null, // remove the `d` prop
  *   }
  * })
  *
  * const useHook2 = createElemPropsHook(useMyModel)((model, ref, elemProps) => {
  *   console.log('useHook2', elemProps)
  *   return {
  *     b: 'useHook2',
- *     c: 'useHook2'
+ *     c: 'useHook2',
+ *     d: 'useHook2',
  *   }
  * })
  *
- * const useHook3 = composeHooks(useHook1, useHook2)
- * const props = composeHooks(model, { c: 'props' })
+ * const useHook3 = composeHooks(
+ *   useHook1, // run last, will have access to `useHook2`'s elemProps, but can remove a prop with `null`
+ *   useHook2 // run first and will override all of `useHook1`'s props
+ * )
+ * const props = useHook3(model, { c: 'props', d: 'props' })
  * console.log('props', props)
  * ```
  *
  * The output would be:
  *
  * ```ts
- * useHook2 {c: 'foo'}
- * useHook1 {b: 'useHook2', c: 'foo'}
- * props {a: 'useHook1', b: 'useHook2', c: 'foo'}
+ * useHook2 {c: 'props', d: 'props'}
+ * useHook1 {b: 'useHook2', c: 'props', d: 'props'}
+ * props {a: 'useHook1', b: 'useHook2', c: 'props', d: null}
  * ```
  */
 export function composeHooks<
-  H1 extends BehaviorHook<any, {}>,
-  H2 extends BehaviorHook<any, {}>,
-  H3 extends BehaviorHook<any, {}>,
-  H4 extends BehaviorHook<any, {}>,
-  H5 extends BehaviorHook<any, {}>,
-  H6 extends BehaviorHook<any, {}>
+  H1 extends BaseHook<any, {}>,
+  H2 extends BaseHook<any, {}>,
+  H3 extends BaseHook<any, {}>,
+  H4 extends BaseHook<any, {}>,
+  H5 extends BaseHook<any, {}>,
+  H6 extends BaseHook<any, {}>,
+  H7 extends BaseHook<any, {}>
 >(
   hook1: H1,
   hook2: H2,
   hook3?: H3,
   hook4?: H4,
   hook5?: H5,
   hook6?: H6,
+  hook7?: H7,
   // TypeScript will only infer up to 6, but the types will still exist for those 6. The rest of the
   // hooks won't add to the interface, but that seems to be an okay fallback
   ...hooks: BehaviorHook<any, any>[]
@@ -939,11 +952,24 @@ export function composeHooks<
       ? H4 extends BaseHook<any, infer O4>
         ? H5 extends BaseHook<any, infer O5>
           ? H6 extends BaseHook<any, infer O6>
-            ? BehaviorHook<M, O1 & O2 & O3 & O4 & O5 & O6>
-            : BehaviorHook<M, O1 & O2 & O3 & O4 & O5>
-          : BehaviorHook<M, O1 & O2 & O3 & O4>
-        : BehaviorHook<M, O1 & O2 & O3>
-      : BehaviorHook<M, O1 & O2>
+            ? H7 extends BaseHook<any, infer O7>
+              ? BehaviorHook<
+                  M,
+                  RemoveNulls<
+                    MergeProps<
+                      O1,
+                      MergeProps<
+                        O2,
+                        MergeProps<O3, MergeProps<O4, MergeProps<O5, MergeProps<O6, O7>>>>
+                      >
+                    >
+                  >
+                >
+              : never
+            : never
+          : never
+        : never
+      : never
     : never
   : never;
 export function composeHooks<M extends Model<any, any>, P extends {}, O extends {}>(

modules/react/common/lib/utils/mergeProps.ts

@@ -1,5 +1,31 @@
 import {mergeCallback} from './mergeCallback';
 
+export type KeysMatching<T, V> = {[K in keyof T]-?: T[K] extends V ? K : never}[keyof T];
+export type RemoveNulls<T> = Omit<T, KeysMatching<T, null>>;
+
+/**
+ * MergeProps will merge keys from `U` over `T` except when the value of `T` is `null`.
+ *
+ * ```ts
+ * MergeProps<
+ *   {foo: string, bar: string, baz: null},
+ *   {foo: string, bar: number, baz: string}
+ * > // { foo: string, bar: number, baz: null }
+ * ```
+ */
+export type MergeProps<T, U> = {
+  // merge keys from both `T` and `U`
+  [K in keyof T | keyof U]: K extends keyof T // test if key is in `T`
+    ? K extends keyof U // test if key is also in `U`
+      ? T[K] extends null // test if `T[K]` is `null`
+        ? null // `K` is in both `T` and `U` and `T[K]` is `null`, so return `null`
+        : U[K] // `K` is in both `T` and `U`, but isn't `null` in `T[K]`, so return `U[K]`
+      : T[K] // `K` is only in `T`, so return `T[K]`
+    : K extends keyof U // K is not in `T`, so test if it is in `U`. This should always match at this point, but there's no "else" in type ternaries
+    ? U[K] // K is only in `U`, so return `U[K]`
+    : never; // We should never get here, but type ternaries need all paths defined. `never` is usually used in these cases
+};
+
 // This file suppresses TS errors that come from merging interfaces of elements that aren't
 // determined within components. Element interfaces are determined only when used, so TS errors
 // aren't even useful here. Things get complicated when merging interfaces of callbacks.
@@ -12,11 +38,11 @@ import {mergeCallback} from './mergeCallback';
  * If `targetProps` has a `null` set, it will remove the prop from the `sourceProps`. This allows
  * passing of props from merged hooks to another without passing out to the final element props.
  */
-export function mergeProps<T extends object, S extends object>(
+export function mergeProps<const T extends object, const S extends object>(
   targetProps: T,
   sourceProps: S
-): S & T {
-  const returnProps = {...targetProps} as S & T;
+): RemoveNulls<MergeProps<T, S>> {
+  const returnProps = {...targetProps} as MergeProps<T, S>;
 
   for (const key in sourceProps) {
     if (sourceProps.hasOwnProperty(key)) {

modules/react/common/spec/components.spec.tsx

@@ -16,6 +16,7 @@ import {
   createSubcomponent,
   createModelHook,
   useModelContext,
+  createElemPropsHook,
 } from '@workday/canvas-kit-react/common';
 
 describe('createComponent', () => {
@@ -427,7 +428,7 @@ describe('composeHooks', () => {
   });
 
   it('should call hook both callbacks', () => {
-    const props = composeHooks(hook1, hook2)(myModel, {}, null) as {onClick: Function};
+    const props = composeHooks(hook1, hook2)(myModel, {}, null) as any as {onClick: Function};
     props.onClick({event: 'foo'});
     expect(spy1).toHaveBeenCalled();
     expect(spy1).toHaveBeenCalledWith({event: 'foo'});
@@ -621,6 +622,25 @@ describe('composeHooks', () => {
 
       expectTypeOf(props).toEqualTypeOf<Expected>();
     });
+
+    it('should compose hooks with conflicting types with null values', () => {
+      const useModel = config => ({state: {}, events: {}});
+      const useHook1 = createElemPropsHook(useModel)(model => ({foo: 'bar', item: null}));
+      const useHook2 = createElemPropsHook(useModel)(model => ({bar: 'baz', item: 'test'}));
+
+      const useHookComposed = composeHooks(useHook1, useHook2);
+      const fakeModel = {state: {}, events: {}};
+
+      const elemProps = useHookComposed(useModel({}), {});
+
+      expect(elemProps).toEqual({foo: 'bar', bar: 'baz', item: null});
+
+      expectTypeOf(elemProps).toHaveProperty('foo');
+      expectTypeOf(elemProps.foo).toBeString();
+      expectTypeOf(elemProps).toHaveProperty('bar');
+      expectTypeOf(elemProps.bar).toBeString();
+      expectTypeOf(elemProps).toEqualTypeOf<{foo: 'bar'; bar: 'baz'}>();
+    });
   });
 });
 

modules/react/common/spec/mergeProps.spec.tsx

@@ -2,6 +2,7 @@
 /** @jsx jsx */
 import {jsx} from '@emotion/react';
 import {render, screen} from '@testing-library/react';
+import {expectTypeOf} from 'expect-type';
 
 import {mergeProps} from '../lib/utils';
 
@@ -25,7 +26,35 @@ describe('mergeProps', () => {
       foo: 'baz',
     };
 
-    expect(mergeProps(target, source)).toEqual({foo: 'baz'});
+    const props = mergeProps(target, source);
+    expect(props).toEqual({foo: 'baz'});
+    expectTypeOf(props).toEqualTypeOf<{foo: string}>();
+  });
+
+  it('should override target props types with source props types', () => {
+    const target = {
+      foo: 'bar',
+    };
+    const source = {
+      foo: 1,
+    };
+
+    const props = mergeProps(target, source);
+    expect(props).toEqual({foo: 1});
+    expectTypeOf(props).toEqualTypeOf<{foo: number}>();
+  });
+
+  it('should override source prop when value of that prop is `null`', () => {
+    const target = {
+      foo: null,
+    };
+    const source = {
+      foo: 'foo',
+    };
+
+    const props = mergeProps(target, source); //?
+    expect(props).toEqual({foo: null});
+    expectTypeOf(props).toEqualTypeOf<{}>();
   });
 
   it('should call both callbacks of the same keys', () => {

modules/styling-transform/lib/styleTransform.ts

@@ -120,7 +120,7 @@ export default function styleTransformer(
       return newNode || ts.visitEachChild(node, visit, context);
     };
 
-    return node => ts.visitNode(node, visit);
+    return (node => ts.visitNode(node, visit)) as ts.Transformer<ts.SourceFile>;
   };
 }