data: Begin adding types, starting with redus-store/meta/actions #32855
Conversation
This file contains 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
sarayourfriend
added
[Type] Code Quality
sarayourfriend
requested review from
jsnajdr,
sirreal,
ciampo,
diegohaz and
tyxla
|
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
sirreal
approved these changes
Jun 22, 2021
| * | ||
| * @return {Object} Action object. | ||
| * @return {{ type: 'START_RESOLUTION', selectorName: string, args: unknown[] }} Action object. |
There was a problem hiding this comment.
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.typeby default like const assertions can, but I don't think there's any way around that.
There was a problem hiding this comment.
So... which do we do? Convert the file to TS and use the const assertion or make the types readonly?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🤷
| }, | ||
| "include": [ | ||
| "src/redux-store/metadata/actions.js" | ||
| ] |
There aren't currently any references being utilized by the types, right? The pattern I've been following is to add to |
|
References are required to ensure projects are built by TypeScript in the correct order. Ideally, it represents the project faithfully from the start, and we'd all of these as references: gutenberg/packages/data/package.json Lines 31 to 36 in 77bc9ca It's true it will work because we're only "compiling" a single file right now that doesn't depend on the types, but it seems preferable to get the setup in place and correct rather than having to deal with things as the introduce problems. Project references are error prone in my experience and it's not always obvious to folks where issues come from. If |
sarayourfriend
force-pushed
the
add/types-data-redux-store-metadata-actions
branch
from
972ed5f
to
0ad2638
Compare
sarayourfriend
force-pushed
the
add/types-data-redux-store-metadata-actions
branch
from
0ad2638
to
2049dcf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Start adding types to
data. Just adds type checking to a simple, low-level module in the package, nothing too complicated.Part of #18838
How has this been tested?
Type checks pass.
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).