docgen: Unwrap wrapped selectors when inferring types of JSDoc params#40236
Merged
docgen: Unwrap wrapped selectors when inferring types of JSDoc params#40236
Conversation
|
Size Change: 0 B Total Size: 1.22 MB ℹ️ View Unchanged
|
Contributor
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
Thank you for the great documentation. Would you mind adding some unit tests to cover this case?
In #40025 we ran into an obstacle when transitioning code into TypeScript, namely that our `docgen` tool is unable to find the associated types for parameters which are documented in JSDoc comments if the parameters are for the returned function from a call to some wrapping function. In this patch we're adding two special cases for selectors that call `createSelector` and `createRegistrySelector` to allow our `docgen` tool to analyze those inner functions which represent the actual selector. Fundamentally we should be asking TypeScript for the inferred types of the function and its parameters but given that we don't have a current mechanism to do that this issue remains a blocker for broader TypeScript work. Because of that we're introducing hard-coded special cases for these common selector wrappers so that we can unblock the TypeScript work without introducing a generic compromise with potentially-harmful side-effects, such as might happen if we were to always return the first argument of a call expression.
dmsnell
force-pushed
the
docgen/unwrap-wrapped-selectors
branch
from
50e09d4 to
a41f044
Compare
Member
Author
Added in 60eb127 ✅ |
sarayourfriend
approved these changes
Apr 15, 2022
Contributor
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
LGTM! Great idea to use the engine to parse actual code instead of just creating AST. It makes the test a lot easier to understand at a glance.
adamziel
reviewed
Apr 22, 2022
| return token.arguments[ 0 ]; | ||
| } | ||
|
|
||
| // createRegistrySelector( ( selector ) => ( state, queryId ) => select( 'core/queries' ).get( queryId ) ); |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What
In #40025 we ran into an obstacle when transitioning code into TypeScript,
namely that our
docgentool is unable to find the associated types forparameters which are documented in JSDoc comments if the parameters are
for the returned function from a call to some wrapping function.
In this patch we're adding two special cases for selectors that call
createSelectorandcreateRegistrySelectorto allow ourdocgentoolto analyze those inner functions which represent the actual selector.
Fundamentally we should be asking TypeScript for the inferred types of
the function and its parameters but given that we don't have a current
mechanism to do that this issue remains a blocker for broader TypeScript
work. Because of that we're introducing hard-coded special cases for
these common selector wrappers so that we can unblock the TypeScript
work without introducing a generic compromise with potentially-harmful
side-effects, such as might happen if we were to always return the
first argument of a call expression.
Why
We don't want to block helpful TypeScript updates until we can rebuild
our documentation generation tooling.
Testing
The easiest way to see the impact of this change is to checkout #40025
and try to run
npm run api-docs:ref. It should fail with nebulous errorsabout improper syntax.
If you merge this branch however, then run
npm build:packages(tore-generate
docgen) and thennpm run api-docs:refagain you shouldnotice that it succeeds and in inspecting the generated
READMEupdatessee that the types for the selectors come through even for those
selectors which are wrapped.
There should be no functional or built-code changes in this PR.