Skip to content

data: Begin adding types, starting with redus-store/meta/actions #32855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sarayourfriend merged 2 commits into from
Jun 23, 2021
Merged

data: Begin adding types, starting with redus-store/meta/actions #32855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d3c916
data: Begin adding types, starting with redus-store/meta/actions
sarayourfriend Jun 21, 2021
2049dcf
Add references
sarayourfriend Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions packages/data/src/redux-store/metadata/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
* Returns an action object used in signalling that selector resolution has
* started.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Arguments to associate for uniqueness.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {unknown[]} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
* @return {{ type: 'START_RESOLUTION', selectorName: string, args: unknown[] }} Action object.
Copy link
Member

@sirreal sirreal Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript features are really nice with action creators. Rather than effectively writing out the object twice, we use a const assertion and let type inference do the work. No need annotate the return type.

export function someAction( arg ) {
  return {
    type: 'SOME_ACTION',
    payload: arg,
  } as const; // <- "const assertion"
}

One thing to consider in the entire data layer is the use of readonly types. We could use mapped types like TypeScript Readonly<Type> utility (shallowly sets readonly), some DeepReadonly implementation, or by adding readonly manually:

Suggested change
* @return {{ type: 'START_RESOLUTION', selectorName: string, args: unknown[] }} Action object.
* @return {{ readonly type: 'START_RESOLUTION', readonly selectorName: string, readonly args: readonly unknown[] }} Action object.

Considerations:

  • The types are more verbose
  • It may be applied inconsistently. There's room for error.
  • Doing it in the action creators themselves would require users to follow the same pattern.
  • This better reflects usage and expectations.
  • We may be able to apply readonly at the framework level in store creation/registration, benefitting more users of the library. This won't provide string literal action.type by default like const assertions can, but I don't think there's any way around that.

Copy link
Contributor Author

@sarayourfriend sarayourfriend Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... which do we do? Convert the file to TS and use the const assertion or make the types readonly?

Copy link
Member

@sirreal sirreal Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a larger consideration than this PR, it's something we should keep in mind especially as we introduce types around Store objects.

My preference is to use as const for action creators. It produces the best result with the lowest effort. A possible downside is that the documentation generator may not handle it well, I'm not sure and haven't tested.

Copy link
Contributor

@ciampo ciampo Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to use as const for action creators. It produces the best result with the lowest effort.

Should we start with this approach and make changes only if necessary?

Copy link
Contributor Author

@sarayourfriend sarayourfriend Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation generator isn't able to infer return types (it doesn't actually run TypeScript compiler, just looks at the static code), they must be explicity declared, so using as const is hindered in that regard. Here these files are not affected by the documentation generator so we can use as const without any problem, but we might not be able to do so in all cases 🤷

*/
export function startResolution( selectorName, args ) {
return {
Expand All @@ -19,10 +19,10 @@ export function startResolution( selectorName, args ) {
* Returns an action object used in signalling that selector resolution has
* completed.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Arguments to associate for uniqueness.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {unknown[]} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
* @return {{ type: 'FINISH_RESOLUTION', selectorName: string, args: unknown[] }} Action object.
*/
export function finishResolution( selectorName, args ) {
return {
Expand All @@ -36,11 +36,11 @@ export function finishResolution( selectorName, args ) {
* Returns an action object used in signalling that a batch of selector resolutions has
* started.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {unknown[]} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
*
* @return {Object} Action object.
* @return {{ type: 'START_RESOLUTIONS', selectorName: string, args: unknown[] }} Action object.
*/
export function startResolutions( selectorName, args ) {
return {
Expand All @@ -54,11 +54,11 @@ export function startResolutions( selectorName, args ) {
* Returns an action object used in signalling that a batch of selector resolutions has
* completed.
*
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {unknown[]} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.
*
* @return {Object} Action object.
* @return {{ type: 'FINISH_RESOLUTIONS', selectorName: string, args: unknown[] }} Action object.
*/
export function finishResolutions( selectorName, args ) {
return {
Expand All @@ -71,10 +71,10 @@ export function finishResolutions( selectorName, args ) {
/**
* Returns an action object used in signalling that we should invalidate the resolution cache.
*
* @param {string} selectorName Name of selector for which resolver should be invalidated.
* @param {Array} args Arguments to associate for uniqueness.
* @param {string} selectorName Name of selector for which resolver should be invalidated.
* @param {unknown[]} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
* @return {{ type: 'INVALIDATE_RESOLUTION', selectorName: string, args: any[] }} Action object.
*/
export function invalidateResolution( selectorName, args ) {
return {
Expand All @@ -88,7 +88,7 @@ export function invalidateResolution( selectorName, args ) {
* Returns an action object used in signalling that the resolution
* should be invalidated.
*
* @return {Object} Action object.
* @return {{ type: 'INVALIDATE_RESOLUTION_FOR_STORE' }} Action object.
*/
export function invalidateResolutionForStore() {
return {
Expand All @@ -103,7 +103,7 @@ export function invalidateResolutionForStore() {
* @param {string} selectorName Name of selector for which all resolvers should
* be invalidated.
*
* @return {Object} Action object.
* @return {{ type: 'INVALIDATE_RESOLUTION_FOR_STORE_SELECTOR', selectorName: string }} Action object.
*/
export function invalidateResolutionForStoreSelector( selectorName ) {
return {
Expand Down
20 changes: 20 additions & 0 deletions packages/data/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"rootDir": "src",
"declarationDir": "build-types",
"noUnusedParameters": false,
},
"references": [
{ "path": "../compose" },
{ "path": "../deprecated" },
{ "path": "../element" },
{ "path": "../is-shallow-equal" },
{ "path": "../priority-queue" },
// Needs to be added once it is typed
// { "path": "../redux-routine" }
],
"include": [
"src/redux-store/metadata/actions.js"
]
Copy link
Member

@sirreal sirreal Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing references.

}
1 change: 1 addition & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
{ "path": "packages/block-editor" },
{ "path": "packages/components" },
{ "path": "packages/compose" },
{ "path": "packages/data" },
{ "path": "packages/date" },
{ "path": "packages/deprecated" },
{ "path": "packages/docgen" },
Expand Down