Data: Improve documentation for new API added around stores#27061
Conversation
gziolo
added
[Package] Data
| import * as metadataSelectors from './metadata/selectors'; | ||
| import * as metadataActions from './metadata/actions'; | ||
|
|
||
| /** @typedef {import('../types').WPDataRegistry} WPDataRegistry */ |
There was a problem hiding this comment.
@sirreal - we have similar usage in other places, but this one is unique as it references a TypeScript file. Does it work correctly with our configuration? Is there a better way to code it in JSDoc?
Aside: I would mind if we were to use TypeScript files for type definitions rather than JSDoc comments but continue using JSDoc only approach for typing codebase 😄
There was a problem hiding this comment.
I did this, I prefer a typescript file for type definition personally, it's easier than big JSDoc typedefs.
There was a problem hiding this comment.
I did this, I prefer a typescript file for type definition personally, it's easier than big JSDoc typedefs.
I know you did, but I added a typedef (sort of proxy) to fix the documentation generated to output the actual type rather than the Unknown type string.
There was a problem hiding this comment.
true, the doc generator should be improved. We might want to try a typescript based one
There was a problem hiding this comment.
I thought this would be fine, but @saramarcondes pointed out to me that it can create a problem in the generated types. They would need to include the types, but .d.ts declaration files will not be included in the compiled types.
We may be able to work around that with some hacks, but the simplest way forward is to change from a declaration file (.d.ts) to a TypeScript file (.ts) which will then be bundled. I'd recommend that approach.
You can confirm this by checking build-types for the stan package, e.g. build-types/store.d.ts:
export function createStoreAtom<T>(subscribe: (listener: () => void) => (() => void), get: () => T, dispatch: (action: any) => void, config?: WPCommonAtomConfig | undefined): import("./types").WPAtom<T>;
export type WPAtom<T> = (registry: import("./types").WPAtomRegistry) => import("./types").WPAtomState<T>;
export type WPCommonAtomConfig = {
id?: string | undefined;
isAsync?: boolean | undefined;
};
//# sourceMappingURL=store.d.ts.mapThis file references ./types, which doesn't exist. There's a Stack Overflow question that provides more detail.
This will create problems in the published types.
@youknowriad @gziolo What do you think?
There was a problem hiding this comment.
so by just renaming the file, it works? Sounds good to me.
There was a problem hiding this comment.
As long as we don't use regular .ts files, it should be fine to rename to make it work 😃
| /** | ||
| * Registers a standard `@wordpress/data` store definition. | ||
| * | ||
| * @param {import('./types').WPDataStore} store Store definition. |
There was a problem hiding this comment.
@nosolosw – is there any way we could improve @wordpress/docgen to ignore the import part so we don't have to use typedef workaround?
|
Size Change: 0 B Total Size: 1.19 MB ℹ️ View Unchanged
|
| const { apiFetch } = wp; | ||
| const { registerStore } = wp.data; | ||
| import apiFetch from '@wordpress/api-fetch'; | ||
| import { registerStore } from '@wordpress/data'; |
There was a problem hiding this comment.
I believe we should remove "registerStore" and "registerGenericStore" from our docs and just use "register" and "createReduxStore"
There was a problem hiding this comment.
Yes, it's a good idea. Let's do it separately as it requires more aggressive changes to README file.
gziolo
force-pushed
the
update/data-store-param-docs
branch
from
dfae322 to
2a3683d
Compare
gziolo
requested review from
ajitbohra,
atimmer,
cameronvoell,
ellatrix,
etoledom,
guarani and
nerrad
as code owners
|
It's ready for final review, there are way too many whitespace changes in The diff mode that ignores them is very helpful: https://github.com/WordPress/gutenberg/pull/27061/files?diff=unified&w=1 |
gziolo
force-pushed
the
update/data-store-param-docs
branch
from
2a3683d to
f5ae263
Compare
3 participants
Description
This PR improves
@wordpress/datadocumentation based on the changes introduced in #26655. Some of the points were raised by @jsnajdr:In a gist, this PR ensures that:
@wordpress/dataana listed in CHANGELOG filewp.dataglobal gets replaced with import statements for@wordpress/datadecide what to do aboutomitstoreConfigfrom@wordpress/core-dataas it was never published to npmHow has this been tested?
npm run test-unitnpm run docs:build