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.

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

wp.data: Split store implementation out from registry. #10289

Merged
merged 14 commits into from
Nov 2, 2018
Merged

wp.data: Split store implementation out from registry. #10289

merged 14 commits into from
Nov 2, 2018

Conversation

coderkevin
Copy link
Contributor

@coderkevin coderkevin commented Oct 2, 2018

Description

The goal of this PR is to split out the implementation of @wordpress/data store implementations to be separate from the registry, opening up the registry for alternative data implementations that can all share a common interface. The ethos behind this is to allow @worpdress/data to be a universal interface for data within Gutenberg and WordPress core, regardless of the implementation of the data system backing it. The benefit of doing this is so existing data systems can be integrated with @wordpress/data for interoperation or transition purposes.

The common interface for data to be registered will be:

registry.registerGenericStore( key, { getSelectors, getActions, subscribe } );
  • getSelectors() will return an object of pre-bound named selector functions.
  • getActions() will return an object of pre-bound functions.
  • subscribe() will behave as a redux store subscribe.

Note: This deprecates the following functions on the registry in favor of using registerStore and registerGenericStore.

  • registerReducer (use registerStore instead)
  • registerSelectors (use registerStore instead)
  • registerActions (use registerStore instead)
  • registerResolvers (use registerStore instead)
  • use (implement using registerGenericStore instead)

How has this been tested?

I purposely left existing tests alone in this PR to ensure that they all run the exact same way as before.
I also added a few tests to verify expectations of registerGenericStore parameter types. No difference in operation should be perceptible.

Types of changes

This is a refactor of the registry, to remove the implicit store implementation from it.
All code and tests should still behave the same for @wordpress/data and the Gutenberg editor.

After the initial interface here is settled, I plan on creating at least one example PR in another repo to validate the operation of using another implementation via registerGenericStore

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@notnownikki
Copy link
Member

How do I use resolvers with this new registration method?

@youknowriad
Copy link
Contributor

@notnownikki It shouldn't change a lot once this is ready. This is basically adding a new register function but the existing one will remain, it will just use this newly introduced function under the hood.

@notnownikki
Copy link
Member

Oh thank goodness for that, resolvers are sweet sweet magic :)

@coderkevin
Copy link
Contributor Author

@youknowriad, @aduth, @gziolo, and others, I think this PR is ready for reviewing. There are other things I'd like to do, like deprecating some functions and the plugin mechanism, then making store configs immutable when added (except to store state, of course), which would simplify things further, but that will take more work to untangle.

I'm satisfied with this as a first step into separating out the interfaces from the implementation, and it should allow alternative stores to be implemented.

@gziolo gziolo added [Package] Data /packages/data [Type] Enhancement A suggestion for improvement. labels Oct 9, 2018
@gziolo gziolo added this to the 4.1 milestone Oct 9, 2018
@gziolo gziolo requested review from aduth, youknowriad and gziolo October 9, 2018 15:03
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

getSelectors() will return an object of pre-bound named selector functions.
getActions() will return an object of pre-bound functions.

To one who is creating a non-Redux-based store, what does "pre-bound" mean to them?

To this and my earlier comment, I'm curious what a minimal example of a custom store might look like. This seems like a good thing to include in README.md anyways.

packages/data/src/registry.js Outdated Show resolved Hide resolved
@gziolo gziolo modified the milestones: 4.1, 4.2 Oct 15, 2018
@gziolo
Copy link
Member

gziolo commented Oct 15, 2018

I'm moving it to 4.2 as we decided that 4.1 should be UI freeze focused. 4.2 is planned to be focused on API freeze and this PR fits perfectly to that description :)

@coderkevin
Copy link
Contributor Author

@youknowriad, @aduth, @gziolo, and others, I have updated the README and added tests for registerGenericStore. Feel free to take a look at both of those for example code of how this will be used. Please review and let me know if you have further questions. Thanks!

packages/data/README.md Outdated Show resolved Hide resolved
packages/data/README.md Outdated Show resolved Hide resolved
packages/data/README.md Outdated Show resolved Hide resolved
@coderkevin coderkevin changed the title WIP: data: Split store implementation out from registry. data: Split store implementation out from registry. Oct 24, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Do you have time to rebase this PR. As we simplified a lot of things in the data module in master.

Also, should we add deprecations for register* functions and use?

@youknowriad
Copy link
Contributor

Moving out of 4.2 for now as it doesn't seem close.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018
@coderkevin
Copy link
Contributor Author

Sorry @youknowriad, I've been out ill for a couple days. I'll get this rebased by tomorrow.

This splits out the implementation of `registerReducer` into a separate
file, with the intention of continuing the rest of the store
implementation. This will make room for other possible implementations
with a common interface.
This moves the implementation of registerSelectors and registerActions
to the namespace-store implementation file.
This untangles registerResolvers, puts more code into the
namespace-store, and organizes the functions upon which it depends.
This commit centralizes the logic for namespace registration, in order
to support `registerStore` better.
`register[Reducer|Actions|Selectors|Resolvers]` now are viewed as
incomplete calls to `registerStore`
This adds the registerGenericStore function and removes the `namespaces`
object in favor of a more generic `stores` object.
This makes `registry.registerGenericStore` accessible from the result of
`createRegistry`, and adds tests for the functionality of it.
This adds information about `registerGenericStore` to the readme with
two examples.
This adds an export for `registerGenericStore` for the defaultRegistry.
While the defaultRegistry may be going away in the future, it's here for
now, so this should be there until it changes.
This deprecates the following functions from the registry:

- registerReducer
- registerSelectors
- registerActions
- registerResolvers
- use

registerStore should be used instead of the above register functions,
and registerGenericStore should be used instead of plugins.
@coderkevin
Copy link
Contributor Author

I have rebased on top of current master for today and added deprecation calls for the following:

  • 'registerReducer'
  • 'registerSelectors'
  • 'registerActions'
  • 'registerResolvers'
  • 'use'

The register functions above should use registerStore instead. And code that calls use should be able to call registerGenericStore instead to implement what it needs.

@coderkevin
Copy link
Contributor Author

@youknowriad, @aduth, @gziolo, and others, may I humbly ask that we try to get this PR merged before any other large changes happen to the registry.js file? It took the better part of a day to get through the rebase on this and I'd prefer not to do that again.

This documents the deprecated.md and CHANGELOG.md files with
information about th wp.data registry deprecations.
It also sets the correct version for the deprecated calls to the next
minor plugin version.
This removes the deprecation of `use` because it's internally
referenced and breaks unit tests. The deprecation of `use` should be
done in a future PR that changes the behavior to not call `use`.
@coderkevin coderkevin changed the title data: Split store implementation out from registry. wp.data: Split store implementation out from registry. Nov 1, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, I'd appreciate a second opinion cc @aduth

packages/data/README.md Show resolved Hide resolved
packages/data/package.json Show resolved Hide resolved
This adds the `wp-deprecated` script as a js script dependency to
`wp-data`.
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This is great work 👍

packages/data/src/test/registry.js Show resolved Hide resolved
@aduth aduth modified the milestones: WordPress 5.0, 4.3 Nov 2, 2018
@aduth aduth merged commit f733526 into WordPress:master Nov 2, 2018
@coderkevin
Copy link
Contributor Author

Thanks @youknowriad and @aduth for the reviews and support! 🎉

@coderkevin coderkevin deleted the update/data-registry-split-implementation branch November 3, 2018 12:42
@aduth aduth mentioned this pull request Nov 27, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants