Skip to content

Commit

Permalink
docgen: Expand createSelector() pattern matching to recognize getDepe…
Browse files Browse the repository at this point in the history
…ndants

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.
  • Loading branch information
dmsnell committed May 6, 2022
1 parent a7bd3be commit c44282d
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 7 deletions.
3 changes: 1 addition & 2 deletions packages/core-data/src/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
41 changes: 36 additions & 5 deletions packages/docgen/lib/get-type-annotation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
Expand Down
62 changes: 62 additions & 0 deletions packages/docgen/test/get-type-annotation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

0 comments on commit c44282d

Please sign in to comment.