Skip to content
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

Data: Document WPDataRegistry properties #16693

Merged
merged 4 commits into from
Jul 26, 2019
Merged

Data: Document WPDataRegistry properties #16693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bcd8ed
Data: Document WPDataRegistry properties
aduth Jul 20, 2019
74f472a
Data: Use WPDataRegistry type where applicable
aduth Jul 20, 2019
e2d517f
Data: Fix WPDataRegistry typedef syntax
aduth Jul 26, 2019
aa80881
Data: Bring WPDataRegistry into scope for typedef
aduth Jul 26, 2019
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
36 changes: 22 additions & 14 deletions packages/data/src/namespace-store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,18 @@ import metadataReducer from './metadata/reducer';
import * as metadataSelectors from './metadata/selectors';
import * as metadataActions from './metadata/actions';

/**
* @typedef {import('../registry').WPDataRegistry} WPDataRegistry
*/

/**
* Creates a namespace object with a store derived from the reducer given.
*
* @param {string} key Identifying string used for namespace and redex dev tools.
* @param {Object} options Contains reducer, actions, selectors, and resolvers.
* @param {Object} registry Registry reference.
* @param {string} key Unique namespace identifier.
* @param {Object} options Registered store options, with properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Type Object is not useful IMHO. I assume this is temporary until the typescript checking makes its way into this file, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Type Object is not useful IMHO. I assume this is temporary until the typescript checking makes its way into this file, yes?

I'd say temporary, yes. I'm not really changing anything here aside from alignment and description. WPDataRegistry is the only type being changed throughout this pull request.

* describing reducer, actions, selectors, and
* resolvers.
* @param {WPDataRegistry} registry Registry reference.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no way for editors to know how to interpret this without importing it in the file as its own typedef.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no way for editors to know how to interpret this without importing it in the file as its own typedef.

Hm, is this what the import is used for in your own pull requests? How does that work with @typedef (vs. your TypeScript definitions)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Precisely.

In javascript, in order to reference types that are defined elsewhere (be it in a typescript definition file, or as standard JSDoc), the types must first be imported in the referencing file as a @typedef. Once imported, you can freely reference the type throughout that file.

Otherwise the editor will not be able to provide type hints or "intellisense" (in VS editors)

*
* @return {Object} Store Object.
*/
Expand Down Expand Up @@ -102,10 +108,11 @@ export default function createNamespace( key, options, registry ) {
/**
* Creates a redux store for a namespace.
*
* @param {string} key Part of the state shape to register the
* selectors for.
* @param {Object} options Registered store options.
* @param {Object} registry Registry reference, for resolver enhancer support.
* @param {string} key Unique namespace identifier.
* @param {Object} options Registered store options, with properties
* describing reducer, actions, selectors, and
* resolvers.
* @param {WPDataRegistry} registry Registry reference.
*
* @return {Object} Newly created redux store.
*/
Expand Down Expand Up @@ -143,15 +150,16 @@ function createReduxStore( key, options, registry ) {
}

/**
* Maps selectors to a redux store.
* Maps selectors to a store.
*
* @param {Object} selectors Selectors to register. Keys will be used as the
* public facing API. Selectors will get passed the
* state as first argument.
* @param {Object} store The redux store to which the selectors should be mapped.
* @param {Object} registry Registry reference.
* @param {Object} selectors Selectors to register. Keys will be used as
* the public facing API. Selectors will get
* passed the state as first argument.
* @param {Object} store The store to which the selectors should be
* mapped.
* @param {WPDataRegistry} registry Registry reference.
*
* @return {Object} Selectors mapped to the redux store provided.
* @return {Object} Selectors mapped to the provided store.
*/
function mapSelectors( selectors, store, registry ) {
const createStateSelector = ( registeredSelector ) => {
Expand Down
22 changes: 16 additions & 6 deletions packages/data/src/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,23 @@ import createCoreDataStore from './store';
/**
* An isolated orchestrator of store registrations.
*
* @typedef {WPDataRegistry}
* @typedef {Object} WPDataRegistry
*
* @property {Function} registerGenericStore
* @property {Function} registerStore
* @property {Function} subscribe
* @property {Function} select
* @property {Function} dispatch
* @property {Function} registerGenericStore Given a namespace key and settings
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest making it the standard for this entire project (and all of WordPress, if possible) to use the tag @prop instead of @property to save on precious line lengths.

It'll help somewhat with the smushed descriptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll help somewhat with the smushed descriptions.

In some respect I appreciate the constraints to encourage being brief with the names and descriptions.

I don't have a strong feeling about @property vs. @prop, but it needs to be made in the documentation standards, which currently lists @prop as unsupported. A topic for a future JavaScript meeting, perhaps?

* object, registers a new generic
* store.
* @property {Function} registerStore Given a namespace key and settings
* object, registers a new namespace
* store.
* @property {Function} subscribe Given a function callback, invokes
Copy link
Contributor

@dsifford dsifford Jul 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Using this property as an example because it's the easiest one to do. IMHO, it would be better to do one of the following options since the type Function is not really helpful or descriptive.

Option 1

/**
 * @callback Subscriber Description of subscriber
 * @param {() => void} callback Description of callback parameter.
 * @return {void}
 */

Option 2

/**
 * @typedef {(callback: () => void) => void} Subscriber Description of subscriber
 */

And then use it on this line as...

/**
 * [...]
 * @prop {Subscriber} registerGenericStore Given a namespace key and settings
 *                                         object, registers a new generic store.
 * [...]
 */

I assume that this is also temporary though until typescript checking makes its way here. So not super important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are those syntaxes standard in JSDoc? It would be a good thing to detail in the documentation standards, I think.

In general, I agree with you. I didn't aim to change the types here, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

I agree that the documentation standards should be combed through for accuracy and, ideally, these standards should be linted automatically. Pretty sure eslint-plugin-jsdoc should be able to do the brunt of this.

* the callback on any change to state
* within any registered store.
* @property {Function} select Given a namespace key, returns an
* object of the store's registered
* selectors.
* @property {Function} dispatch Given a namespace key, returns an
* object of the store's registered
* action dispatchers.
*/

/**
Expand Down
10 changes: 6 additions & 4 deletions packages/data/src/resolvers-cache-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
import { get } from 'lodash';

/**
* creates a middleware handling resolvers cache invalidation.
* Creates a middleware handling resolvers cache invalidation.
*
* @param {Object} registry
* @param {string} reducerKey
* @param {WPDataRegistry} registry The registry reference for which to create
* the middleware.
* @param {string} reducerKey The namespace for which to create the
* middleware.
*
* @return {function} middleware
* @return {Function} Middleware function.
*/
const createResolversCacheMiddleware = ( registry, reducerKey ) => () => ( next ) => ( action ) => {
const resolvers = registry.select( 'core/data' ).getCachedResolvers( reducerKey );
Expand Down