Convert core-data selectors from jsDoc annotations into TypeScript type signatures #40025

adamziel · 2022-04-04T12:11:52Z

What?

This is a subset of #39025 – a mega branch that proposes TypeScript signatures for all @wordpress/core-data selectors.

This PR removes the jsDoc annotations like @param {Object} and @return {string} in favor of their TypeScript counterparts. This is the first step towards introducing exhaustive type signatures after #40024 is merged.

There are two shortcomings for now:

~~The doc tool that handles START TOKEN(Autogenerated selectors fails. Apparently this is the first instance of mixing it with TypeScript files in the repo.~~ (edit: should be fixed by docgen: Unwrap wrapped selectors when inferring types of JSDoc params #40236)
createRegistrySelector isn't typed at the moment, which means that calling it like const mySelector = createRegistrySelector( select => ( state: State ) => null ) produces a mySelector variable of type any. We could solve it here by adding a .d.ts file to provide the missing signature, or we could solve this problem in @wordpress/data instead.

Testing Instructions

Confirm the checks are green and that no entity configuration got changed when I split the large array into atomic declarations. The changes here should only affect the type system so there is nothing to test in the browser.

cc @dmsnell @jsnajdr @youknowriad @sarayourfriend @getdave @draganescu @scruffian

dmsnell · 2022-04-04T23:18:37Z

I'm confused by this: why create new types out of band instead of adding type annotation to the actual selectors file?

adamziel · 2022-04-05T09:45:12Z

@dmsnell I just started this one without realizing selectors.js can now be easily renamed to selectors.ts. I had a much harder time doing that last month :-) Let's rebase and apply these changes directly.

adamziel · 2022-04-05T11:37:55Z

Ah yes, our doc building tool throw an error when the TS signatures are mixed with jsDoc:

Error: Command failed with exit code 1: "core/plugins/gutenberg/node_modules/.bin/docgen" packages/core-data/src/selectors.ts --output docs/reference-guides/data/data-core.md --to-token --use-token "Autogenerated selectors|../../../packages/core-data/src/selectors.ts" --ignore "/unstable|experimental/i"

TypeError: Cannot read property '0' of undefined
    at makeError (core/plugins/gutenberg/node_modules/execa/lib/error.js:59:11)
    at handlePromise (core/plugins/gutenberg/node_modules/execa/index.js:114:26)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async Transform.<anonymous> (core/plugins/gutenberg/bin/api-docs/update-api-docs.js:219:5) {
  shortMessage: 'Command failed with exit code 1: "core/plugins/gutenberg/node_modules/.bin/docgen" packages/core-data/src/selectors.ts --output docs/reference-guides/data/data-core.md --to-token --use-token "Autogenerated selectors|../../../packages/core-data/src/selectors.ts" --ignore "/unstable|experimental/i"',
  command: '"core/plugins/gutenberg/node_modules/.bin/docgen" packages/core-data/src/selectors.ts --output docs/reference-guides/data/data-core.md --to-token --use-token "Autogenerated selectors|../../../packages/core-data/src/selectors.ts" --ignore "/unstable|experimental/i"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: "\nTypeError: Cannot read property '0' of undefined",
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}

I'll just convert all the jsDoc annotations into TypeScript ones, then.

adamziel · 2022-04-05T12:11:02Z

It seems like it's not about mixing the jsDoc annotations with the TypeScript syntax. It's about the TypeScript syntax. I converted the entire selectors.ts file to rely on it, yet the error persists. A quick grep tells me this is the first attempt to use autogenerated annotations with a .ts file:

$ git grep 'START TOKEN' | grep '\.ts'
docs/reference-guides/data/data-core.md:<!-- START TOKEN(Autogenerated selectors|../../../packages/core-data/src/selectors.ts) -->
packages/core-data/README.md:<!-- START TOKEN(Autogenerated selectors|src/selectors.ts) -->

dmsnell · 2022-04-11T20:41:35Z

A quick grep tells me this is the first attempt to use autogenerated annotations with a .ts file:

That thing keeps giving us more and more trouble; @mirka recently raised an issue for React components wanting to add descriptions to arguments inline.

/** Does a thing */
function ({
	/** Name of thing */
	name: string,

	/** How many times you want to do the thing. */
	count: number,
}) => { … }

cc: @sarayourfriend. I'm not sure what to do because I don't want to divert energy to the docs generation, but I feel like this is going to hit us more and more as we move to TypeScript

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 · 2022-04-11T23:38:16Z

The main issue with docgen here is that we've decoupled the descriptions from the types and for wrapped functions that means our parameters are kind of hidden inside the call to the wrapper (createSelector and createRegistrySelector).

In #40236 I've proposed a hard-coded solution to docgen until we can rearchitect it to poll TypeScript for the type information. I think it's worth adding special-cases for selectors in the meantime; that would unblock this PR and other selectors.ts changes we'll be making soon.

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.

…#40236) 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.

… types

…pe is introduced

packages/core-data/src/selectors.ts

…ess/gutenberg into ts/core-data-selectors-signatures

… branch

adamziel · 2022-04-25T10:36:24Z

// actual types of `state` and `n` are bound by `Selector<…>`
const getBooks: Selector<Book[] | null, [name: string]> = (state, n) => …

@dmsnell Hm it is quite clever, still I'd rather just call that argument name, otherwise I'm not sure what it means as I read the code.

"If you are writing a selector, use `const mySelector: Selector = " and then have everything auto-complete.

This would be pretty great! And I think we'll get there when integrating the types from @wordpress/core-data with the ones from @wordpress/data. Perhaps that won't be the first iteration, as you mentioned, but I would really like to make these selectors aware of things like:

I belong to this store
Therefore, the state has a certain type
Also, there are these other selectors I can refer to
I need to pass the state if I call them directly
But I don't need to do that when they're thunks

I don't like using these lengthy infer traps, but I believe that's the way here.

adamziel · 2022-04-25T11:50:59Z

packages/core-data/README.md

 -   _kind_ `string`: Entity kind.
 -   _name_ `string`: Entity name.
-   _recordId_ `string`: Record's id.
+-   _recordId_ `RecordKey`: Record's id.


I don't like how it says RecordKey here, I'd rather have it say string | number

why not? because of the auto-generated documentation?

if that's the case I don't think we can care too much about this problem unless and until we radically change our docs. this is going to be a problem with more and more TypeScript code since this is a win for the types but masks the actual type in the docs. we need hyper-links in our docs to jump to and provide a tooltip description for the types like RecordKey here.

unless we get those hyperlinks/tooltips I don't know if there's a good workaround that we should chase.

I just don't like how it doesn't tell me what a RecordKey is. How do I construct this type as a user of this code? You nailed it here:

we need hyper-links in our docs to jump to and provide a tooltip description for the types like RecordKey here.

Once we have that, we're golden. I don't think it needs to be necessarily hard, too. Perhaps there are even existing documentation tools that do that and that wouldn't be too hard to leverage.

packages/core-data/README.md

adamziel · 2022-04-25T12:46:48Z

@dmsnell Yay, all checks are finally green here. Thank you so much for your help with the documentation tools! I'd say this is ready to be reviewed.

dmsnell

it is quite clever, still I'd rather just call that argument name, otherwise I'm not sure what it means as I read the code.

it wasn't the point of the work, just a side-effect. style preferences aside, some languages offer this as a first-class element to the language as a way of giving more appropriate names to parameters from the outside and inside.

for example,

function compute( $raw_value: value ) {
    $value = clean( $raw_value );
    …
}

Again, not a big deal or the goal, just a side-effect that has its uses.

Things mostly look good here. There's a lot of room for improvement which I think is fine to do in follow-up work.

Get rid of Object
Cleanup use of RecordKey
Expand State
Audit any | ? types.

The any | ? types could honestly go in now before merging and it would be good. I get the purpose of any | null (or at least I can think of a reasonable use-case) but I don't think it warrants the oddity in the type. A stop-gap could be the use of a nullable type like Maybe or Optional.

type Optional<T> = T | null;

And with that we still get any | null if we do Optional<any> but at least we can see that it was intentional and not an oversight.

The state parameter in there for docgen is the other thing I'd like to double-check before merge. Is that still necessary? If it is then please go ahead and merge; if it isn't let's get that out before merging.

Either way, thanks for moving on this very large task. Can't wait until we're getting to the even-more exciting bits.

docs/reference-guides/data/data-core.md

packages/core-data/README.md

packages/core-data/src/selectors.ts

dmsnell

Forgot to mark as approved in my submission…

…ess/gutenberg into ts/core-data-selectors-signatures

adamziel · 2022-05-05T14:17:42Z

it wasn't the point of the work, just a side-effect. style preferences aside, some languages offer this as a first-class element to the language as a way of giving more appropriate names to parameters from the outside and inside.

Thank you for explaining, I misunderstood your intention. It is a nice feature for sure, and it could come handy for our work here.

Also, I've addressed all your notes, included the additional State types – let's get this one in and address any follow-up work in a follow-up PR.

…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.

manzoorwanijk · 2024-12-17T10:36:40Z

packages/core-data/src/selectors.ts

 */
-export function getCurrentUser( state ) {
+export function getCurrentUser( state: State ): User< 'edit' > {


The context here should be 'view' instead of 'edit', otherwise it will set the wrong types/properties which are not available in current user object, for example, user.capabilities.

Created #68045 to fix it.

adamziel added [Type] Code Quality Issues or PRs that relate to code quality [Package] Core data /packages/core-data Developer Experience Ideas about improving block and theme developer experience labels Apr 4, 2022

adamziel requested a review from nerrad as a code owner April 4, 2022 12:11

adamziel self-assigned this Apr 4, 2022

adamziel requested a review from dmsnell April 4, 2022 12:12

adamziel mentioned this pull request Apr 4, 2022

TypeScript signatures for core-data selectors #39025

Closed

adamziel force-pushed the ts/core-data-selectors-signatures branch from 0f1e344 to 759d009 Compare April 5, 2022 11:37

adamziel changed the title ~~Add a subset of core-data selectors TypeScript signatures~~ Convert core-data selectors from jsDoc annotations into TypeScript type signatures Apr 5, 2022

dmsnell mentioned this pull request Apr 11, 2022

docgen: Unwrap wrapped selectors when inferring types of JSDoc params #40236

Merged

adamziel added 5 commits April 22, 2022 15:23

Add signatures for selectors that have no dependency on Kind and Name…

c999fb9

… types

Inline the signatures into selectors.ts

97d4cba

Convert selectors.ts from jsDoc annotations to TypeScript annotations

a98bbd7

Label the "any" type as EntityRecord until the actual EntityRecord ty…

bd7a5d1

…pe is introduced

Add the missing : State type annotatoin

85df8a7

adamziel force-pushed the ts/core-data-selectors-signatures branch from 3a37e20 to 85df8a7 Compare April 22, 2022 13:23

adamziel commented Apr 22, 2022

View reviewed changes

packages/core-data/src/selectors.ts Outdated Show resolved Hide resolved

Update packages/core-data/src/selectors.ts

ac30b42

adamziel commented Apr 22, 2022

View reviewed changes

packages/core-data/src/selectors.ts Outdated Show resolved Hide resolved

Update packages/core-data/src/selectors.ts

1654a6f

adamziel commented Apr 22, 2022

View reviewed changes

packages/core-data/src/selectors.ts Outdated Show resolved Hide resolved

adamziel added 3 commits April 25, 2022 12:27

Lint the selectors.ts file and refresh the documentation

ef2a381

Merge branch 'ts/core-data-selectors-signatures' of github.com:WordPr…

40764be

…ess/gutenberg into ts/core-data-selectors-signatures

Lint and regenerate the documentation after merging with the upstream…

5789cfa

… branch

adamziel requested review from ajitbohra, mkaz, ryanwelcher and juanmaguitar as code owners April 25, 2022 10:29

adamziel commented Apr 25, 2022

View reviewed changes

packages/core-data/README.md Show resolved Hide resolved

dmsnell reviewed May 4, 2022

View reviewed changes

dmsnell approved these changes May 4, 2022

View reviewed changes

adamziel added 6 commits May 5, 2022 14:10

Add Optional<> type to substitute for any | undefined

0ebcca0

Add Optional<> type to substitute for any | undefined

6791f85

Add an approximate implementation of the State type

a486855

Merge branch 'ts/core-data-selectors-signatures' of github.com:WordPr…

c9e4a89

…ess/gutenberg into ts/core-data-selectors-signatures

Use RecordKey in canUser signature

0466bd4

Use RecordKey in all signatures expecting a record id

63274af

adamziel force-pushed the ts/core-data-selectors-signatures branch from 0fb2ada to 63274af Compare May 5, 2022 13:24

adamziel merged commit 4c38108 into trunk May 5, 2022

adamziel deleted the ts/core-data-selectors-signatures branch May 5, 2022 14:18

github-actions bot added this to the Gutenberg 13.3 milestone May 5, 2022

dmsnell mentioned this pull request May 5, 2022

docgen: Expand createSelector() pattern matching to recognize getDependants #40865

Open

manzoorwanijk reviewed Dec 17, 2024

View reviewed changes

manzoorwanijk mentioned this pull request Dec 17, 2024

[core-data]: Fix TS types for user object #68045

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert core-data selectors from jsDoc annotations into TypeScript type signatures #40025

Convert core-data selectors from jsDoc annotations into TypeScript type signatures #40025

adamziel commented Apr 4, 2022 •

edited by dmsnell

Loading

dmsnell commented Apr 4, 2022

adamziel commented Apr 5, 2022 •

edited

Loading

adamziel commented Apr 5, 2022 •

edited

Loading

adamziel commented Apr 5, 2022 •

edited

Loading

dmsnell commented Apr 11, 2022

dmsnell commented Apr 11, 2022

adamziel commented Apr 25, 2022 •

edited

Loading

adamziel Apr 25, 2022

dmsnell May 4, 2022

adamziel May 5, 2022

adamziel commented Apr 25, 2022

dmsnell left a comment

dmsnell left a comment

adamziel commented May 5, 2022

manzoorwanijk Dec 17, 2024

manzoorwanijk Dec 17, 2024

Convert core-data selectors from jsDoc annotations into TypeScript type signatures #40025

Convert core-data selectors from jsDoc annotations into TypeScript type signatures #40025

Conversation

adamziel commented Apr 4, 2022 • edited by dmsnell Loading

What?

Testing Instructions

dmsnell commented Apr 4, 2022

adamziel commented Apr 5, 2022 • edited Loading

adamziel commented Apr 5, 2022 • edited Loading

adamziel commented Apr 5, 2022 • edited Loading

dmsnell commented Apr 11, 2022

dmsnell commented Apr 11, 2022

adamziel commented Apr 25, 2022 • edited Loading

adamziel Apr 25, 2022

Choose a reason for hiding this comment

dmsnell May 4, 2022

Choose a reason for hiding this comment

adamziel May 5, 2022

Choose a reason for hiding this comment

adamziel commented Apr 25, 2022

dmsnell left a comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

adamziel commented May 5, 2022

manzoorwanijk Dec 17, 2024

Choose a reason for hiding this comment

manzoorwanijk Dec 17, 2024

Choose a reason for hiding this comment

adamziel commented Apr 4, 2022 •

edited by dmsnell

Loading

adamziel commented Apr 5, 2022 •

edited

Loading

adamziel commented Apr 5, 2022 •

edited

Loading

adamziel commented Apr 5, 2022 •

edited

Loading

adamziel commented Apr 25, 2022 •

edited

Loading