fix: Fix mergeProps, createElemPropsHook, and composeHooks types #2980
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,6 @@ | |
| "fs-extra": "^10.0.0", | ||
| "glob": "^7.1.6", | ||
| "mkdirp": "^1.0.3", | ||
| "typescript": "5.0" | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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`. | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import React from 'react'; | ||
| import {assert} from './assert'; | ||
| import {memoize} from './memoize'; | ||
| 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) => | ||
| <const PO extends {}, const PI extends {}>( | ||
|
There was a problem hiding this comment. it's not happy about these const in here for some reason There was a problem hiding this comment. The dependency checker doesn't understand the new syntax... It uses an AST parser to function. |
||
| 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 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', | ||
| * d: null, // remove the `d` prop | ||
| * } | ||
| * }) | ||
| * | ||
| * const useHook2 = createElemPropsHook(useMyModel)((model, ref, elemProps) => { | ||
| * console.log('useHook2', elemProps) | ||
| * return { | ||
| * b: 'useHook2', | ||
| * c: 'useHook2', | ||
| * d: 'useHook2', | ||
| * } | ||
| * }) | ||
| * | ||
| * 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: 'props', d: 'props'} | ||
| * useHook1 {b: 'useHook2', c: 'props', d: 'props'} | ||
| * props {a: 'useHook1', b: 'useHook2', c: 'props', d: null} | ||
| * ``` | ||
| */ | ||
| export function composeHooks< | ||
| 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> | ||
| ? 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 {}>( | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<const T extends object, const S extends object>( | ||
| targetProps: T, | ||
| sourceProps: S | ||
| ): RemoveNulls<MergeProps<T, S>> { | ||
| const returnProps = {...targetProps} as MergeProps<T, S>; | ||
|
|
||
| for (const key in sourceProps) { | ||
| if (sourceProps.hasOwnProperty(key)) { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 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'}>(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', | ||
| }; | ||
|
|
||
| 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', () => { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,7 +120,7 @@ export default function styleTransformer( | |
| return newNode || ts.visitEachChild(node, visit, context); | ||
| }; | ||
|
|
||
| return (node => ts.visitNode(node, visit)) as ts.Transformer<ts.SourceFile>; | ||
| }; | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.