DataViews: Enable types #61185
Merged
DataViews: Enable types #61185
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
youknowriad
added
the
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
commented
Apr 29, 2024
| isPrimary?: boolean; | ||
| } | ||
|
|
||
| export interface Field { |
There was a problem hiding this comment.
I tried looking at all of our existing "fields" to define this. It might not be 100% accurate yet. We're also lacking the enumerations for available types and operators but it's a decent start.
There was a problem hiding this comment.
I'm not familiar with this package so this may be off base.
There's an alternative to defining a single interface that all the instances "fit," which is to make this a union of very specific types. If there's a specific set of known fields, a discriminated union can be more expressive and provide much more precise and relevant to developers working with the code.
// Instead of
interface Field {
id: string;
getValue?: ( obj: any ) => any;
}
// A union can be more expressive:
type FieldUnion =
| { id: "countField"; getValue( field: { items: Array<any> } ) => number; }
| { id: "dateField"; getValue( field: { date: Date } ) => string; };
Something to consider. I also wrote a longer form post about the idea.
youknowriad
commented
Apr 29, 2024
| /** | ||
| * The maximum width of the field column. | ||
| */ | ||
| minWidth?: string | number; |
There was a problem hiding this comment.
We have three different "width" properties. It felt too much for me. Do we really need all of these.
youknowriad
commented
Apr 29, 2024
| /** | ||
| * The label of the field. Defaults to the id. | ||
| */ | ||
| header?: string; |
There was a problem hiding this comment.
I think we should rename this one "label". header is too "table" specific wording.
youknowriad
commented
Apr 29, 2024
oandregal
reviewed
Apr 29, 2024
oandregal
reviewed
Apr 29, 2024
oandregal
reviewed
Apr 29, 2024
| /** | ||
| * Apply default values and normalize the fields config. | ||
| * | ||
| * @param {Object[]} fields Raw Fields. | ||
| * @return {Object[]} Normalized fields. | ||
| * @param fields Fields config. |
There was a problem hiding this comment.
no, in typescript, it's not needed because it's actually defined as part of the code.
There was a problem hiding this comment.
Oh, I hadn't noticed the file had been switched to TS. Do we need to switch our code to TypeScript for us to use the TS types in the docs? I thought we could use them in the JSDoc directly.
There was a problem hiding this comment.
@oandregal you can use them in JSDocs. I switched to typescript extension because I didn't want to list all the JS files that are fully type-checked in tsconfig file. the tsconfig file only targets all "ts" files for now.
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
oandregal
reviewed
Apr 29, 2024
| @@ -0,0 +1,20 @@ | |||
| { | |||
There was a problem hiding this comment.
According to https://github.com/WordPress/gutenberg/blob/add/typescript-dataviews/packages/README.md?plain=1#L196 this looks fine.
There was a problem hiding this comment.
Came here to suggest that we should add a types: "build-types" in dataviews' package.json file, but it turns out we already have it. Not sure what whether there's any side-effects this could have (not having types vs having types after this PR), but thought worth mentioning it.
There was a problem hiding this comment.
No important impact aside that third-party devs using the package will now see some types appearing (autocompletion and all) when they use the package. Previously the build-types folder was empty, now it has the types defined in this PR.
There was a problem hiding this comment.
The @wordpress/dataviews/build-types/ folder has the proper data (types + TS files).
Though I'm a bit confused. I wanted to see what we had previously, so I went to trunk and did npm install && npm run build. Prior to that, I removed everything (tried in a few different ways: manually rm -rf node_modules, as well as other options npm run distclean or npm run clean:*). The @wordpress/dataviews/build-types folder contained the types and TS files from this PR 😕 I presume there's a cache somewhere I'm not clearing. It'd be nice to learn how to clear this. Other than that, I suspect it's working as expected.
There was a problem hiding this comment.
You're right I have the same issue. I think the problem is that "dataviews" is not on the root tsconfig.json file in trunk which means when running npm run clean:package-types, typescript doesn't touch it at all, it's not aware of its existence so it's not cleared.
|
The function docs are fine in my editor (VSCode): 
|
|
One reason this could be useful for us is to auto-generate the documentation for DataViews. I looked around and haven't found any example, so I tried this (doing it like we'd do any other exported entity). It doesn't seem to work:
export {default as FieldAPI} from './types';
<!-- START TOKEN(Autogenerated API docs) -->
<!-- END TOKEN(Autogenerated API docs) -->
+<!-- START TOKEN(Autogenerated API docs) -->
+
+#### DataViews
+
+Undocumented declaration.
+
+#### FieldAPI
+
+Undocumented declaration.
+
+#### filterSortAndPaginate
+
+Applies the filtering, sorting and pagination to the raw data based on the view configuration.
+
+_Parameters_
+
+- _data_ `any[]`: Raw data.
+- _view_ `Object`: View config.
+- _fields_ `Object[]`: Fields config.
+
+_Returns_
+
+- `Object`: { data: any\[], paginationInfo: { totalItems: number, totalPages: number } }
+
+#### VIEW_LAYOUTS
+
+Undocumented declaration.
+
+<!-- END TOKEN(Autogenerated API docs) -->
|
|
I don't see how this is useful to us right now, but I'm not against trying until we discover whether it is at some point. |
oandregal
approved these changes
Apr 29, 2024
I actually think this is very useful for us right now, we can stop guessing where we need to update code, when we want to change something in the API itself. The tooling will show us exactly where to change usage. (for instance when removing "width"). but for this to be very effective, we need to expand the types to more files in the package. But I agree entirely about the documentation aspect. I'd love to be able to generate the documentation for the "main" types (fields, views) automatically. It can be helpful for the Block API elsewhere as well... |
|
Thanks for the review |
sirreal
reviewed
Apr 29, 2024
Comment on lines
+4
to
+19
| "compilerOptions": { | ||
| "rootDir": "src", | ||
| "declarationDir": "build-types" | ||
| }, | ||
| "references": [ | ||
| { "path": "../a11y" }, | ||
| { "path": "../components" }, | ||
| { "path": "../compose" }, | ||
| { "path": "../element" }, | ||
| { "path": "../i18n" }, | ||
| { "path": "../icons" }, | ||
| { "path": "../keycodes" }, | ||
| { "path": "../primitives" }, | ||
| { "path": "../private-apis" } | ||
| ], | ||
| "include": [ "src/**/*.ts", "src/**/*.tsx" ] |
There was a problem hiding this comment.
This seems like the preferred way of including src but not typechecking *.js files:
Suggested change
| "compilerOptions": { | |
| "rootDir": "src", | |
| "declarationDir": "build-types" | |
| }, | |
| "references": [ | |
| { "path": "../a11y" }, | |
| { "path": "../components" }, | |
| { "path": "../compose" }, | |
| { "path": "../element" }, | |
| { "path": "../i18n" }, | |
| { "path": "../icons" }, | |
| { "path": "../keycodes" }, | |
| { "path": "../primitives" }, | |
| { "path": "../private-apis" } | |
| ], | |
| "include": [ "src/**/*.ts", "src/**/*.tsx" ] | |
| "compilerOptions": { | |
| "rootDir": "src", | |
| "declarationDir": "build-types", | |
| "checkJs": false | |
| }, | |
| "references": [ | |
| { "path": "../a11y" }, | |
| { "path": "../components" }, | |
| { "path": "../compose" }, | |
| { "path": "../element" }, | |
| { "path": "../i18n" }, | |
| { "path": "../icons" }, | |
| { "path": "../keycodes" }, | |
| { "path": "../primitives" }, | |
| { "path": "../private-apis" } | |
| ], | |
| "include": [ "src" ] |
I think there's an ongoing problem with react and ariakit types that I expect to be fixed by #60796. Sometimes I get errors like this:
node_modules/@ariakit/react-core/cjs/tooltip/tooltip-provider.d.ts:14:100 - error TS2694: Namespace '"…/node_modules/@types/react/jsx-runtime"' has no exported member 'JSX'.
14 export declare function TooltipProvider(props?: TooltipProviderProps): import("react/jsx-runtime").JSX.Element;
We can work around that in this PR if its a problem by adding "skipLibCheck": true to the compilerOptions.
| isPrimary?: boolean; | ||
| } | ||
|
|
||
| export interface Field { |
There was a problem hiding this comment.
I'm not familiar with this package so this may be off base.
There's an alternative to defining a single interface that all the instances "fit," which is to make this a union of very specific types. If there's a specific set of known fields, a discriminated union can be more expressive and provide much more precise and relevant to developers working with the code.
// Instead of
interface Field {
id: string;
getValue?: ( obj: any ) => any;
}
// A union can be more expressive:
type FieldUnion =
| { id: "countField"; getValue( field: { items: Array<any> } ) => number; }
| { id: "dateField"; getValue( field: { date: Date } ) => string; };
Something to consider. I also wrote a longer form post about the idea.
This was referenced Apr 30, 2024
This was referenced May 14, 2024
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.
Related #55083
What?
The DataViews package is a types heavy package. There's a lot of structures that need to be documented properly. So this PR introduces types and typescript to the package. It's not exhaustive by any means, The idea is to add the config and start typing bit by bit. This one adds a "Field" type which is one of the most important ones and only types a single file in the package. We can expand the coverage in follow-ups.
Testing instructions
There's no functional change, it's essentially a code quality + documentation change.