From b8e9889088430319a252c746049d820696e07e02 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Thu, 5 May 2022 15:44:21 -0700 Subject: [PATCH] docgen: Expand createSelector() pattern matching to recognize getDependants In #40025 we ran into a situation where a `createSelector()` selector is building an opaque value memoized on state values as dependencies. While this is an unexpected use of a selector it's legitimate and required leaving some additional code and an explanatory comment in order to avoid breaking the `docgen` process. In this patch we're adding recognition for that second argument to the `createSelector()` function, the `getDependants()` function, and if that function has more parameters than the `selector` itself we'll cheat and act like its parameters were listed on the selector. This will likely only happen in practice when the selector ignores `state` but it's pluasible someone might go further and use other inputs in the dependency selection but ignore them on the actual state creation. --- packages/core-data/src/selectors.ts | 3 +- packages/docgen/lib/get-type-annotation.js | 41 ++++++++++++-- packages/docgen/test/get-type-annotation.js | 62 +++++++++++++++++++++ 3 files changed, 99 insertions(+), 7 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index ee1b4df38416d5..208360b9b9fd78 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -1082,8 +1082,7 @@ export const hasFetchedAutosaves = createRegistrySelector( * @return A value whose reference will change only when an edit occurs. */ export const getReferenceByDistinctEdits = createSelector( - // This unused state argument is listed here for the documentation generating tool (docgen). - ( state: State ) => [], + () => [], ( state: State ) => [ state.undo.length, state.undo.offset, diff --git a/packages/docgen/lib/get-type-annotation.js b/packages/docgen/lib/get-type-annotation.js index f3afa9058f3520..4e009b29690eb2 100644 --- a/packages/docgen/lib/get-type-annotation.js +++ b/packages/docgen/lib/get-type-annotation.js @@ -409,16 +409,47 @@ function unwrapWrappedSelectors( token ) { if ( babelTypes.isCallExpression( token ) ) { // createSelector( ( state, queryId ) => state.queries[ queryId ] ); - // \--------------------------------------------/ CallExpression.arguments[0] + // \--------------------------------------------/ selector if ( token.callee.name === 'createSelector' ) { - return token.arguments[ 0 ]; + const [ selector, getDependants ] = token.arguments; + + /* + * A special and awkward case arises when we're using a selector + * as a flag indicating if dependencies have changed. + * + * createSelector( () => [], ( state ) => [ state.dependents ] ); + * \------/ selector + * \-------------------------------/ getDependants + * + * In these cases we'll use the argument list from `getSelector` + * even though that will result in wrong location information for + * the chosen argument nodes. This is fine because `docgen` doens't + * currently use that location information but if it starts to + * we'll need to update this. + * + * The arguments for `gepDependants` must be a strict subset of the + * arguments for the selector, but `state` is _always_ an implicit + * argument provided by the system, so we always have to have at + * least that. If both functions have no arguments it would imply + * there's no point in making them a selector. + */ + const params = + getDependants && + getDependants.params && + getDependants.params.length > selector.params.length + ? getDependants.params + : selector.params; + + return { ...selector, params }; } // createRegistrySelector( ( selector ) => ( state, queryId ) => select( 'core/queries' ).get( queryId ) ); - // \-----------------------------------------------------------/ CallExpression.arguments[0].body - // \---------------------------------------------------------------------------/ CallExpression.arguments[0] + // \-----------------------------------------------------------/ selector.body + // \---------------------------------------------------------------------------/ selector if ( token.callee.name === 'createRegistrySelector' ) { - return token.arguments[ 0 ].body; + const [ registrySelector ] = token.arguments; + + return registrySelector.body; } } } diff --git a/packages/docgen/test/get-type-annotation.js b/packages/docgen/test/get-type-annotation.js index 13ff923f229906..9eaf3f75e25b2f 100644 --- a/packages/docgen/test/get-type-annotation.js +++ b/packages/docgen/test/get-type-annotation.js @@ -415,6 +415,68 @@ describe( 'Type annotations', () => { ).toBe( 'string[]' ); } ); + it( 'should lean on `getDependants` if `selector` in `createSelector` is missing args', () => { + const { tokens } = engine( + 'test.ts', + `/** + * Returns a reference that changes only when the dependencies do. + * + * @param state - stores all the things + */ + export const getCountReference = createSelector( () => [], ( state: State ) => [ state.items.length ] ); + ` + ); + + expect( + getTypeAnnotation( + { tag: 'param', name: 'state' }, + tokens[ 0 ], + 0 + ) + ).toBe( 'State' ); + } ); + + it( 'should ignore `getDependants` if `selector` in `createSelector` has more arguments', () => { + const { tokens } = engine( + 'test.ts', + `/** + * Returns a reference that changes only when the dependencies do. + * + * @param state - stores all the things + */ + export const getCountReference = createSelector( + ( state: State, id: number ) => [], + ( estate: EState ) => [ estate.items.length ] + ); + ` + ); + + expect( + getTypeAnnotation( + { tag: 'param', name: 'state' }, + tokens[ 0 ], + 0 + ) + ).toBe( 'State' ); + } ); + + it( 'should ignore `getDependants` if everything in `createSelector` ignores `state`', () => { + expect( () => + engine( + 'test.ts', + `/** + * Returns a reference that changes only when the dependencies do. + * + * @param state - stores all the things + */ + export const getCountReference = createSelector( () => [], () => [] ); + ` + ) + ).toThrow( + /Could not find corresponding parameter token for documented parameter/ + ); + } ); + it( 'should find types for inner function with `createRegistrySelector`', () => { const { tokens } = engine( 'test.ts',