Refactor the core/data store to be independent from the registry #14634

youknowriad · 2019-03-26T11:32:56Z

There's a lot in this PR but I don't know if it's breakable into smaller items. The main ideas here:

Bake-in the "core/data" store as a reducer embedded in any namespace store
Build a "virtual" store added to all registries but proxify its selectors/actions per store
Bake the controls plugin in the namespace stores (it wasn't possible to keep it separate and bake "core/data")

This is not ready as I didn't check the tests yet.

nerrad

I like the direction of this.

Build a "virtual" store added to all registries but proxify its selectors/actions per store

I'm assuming this refers to the core/data generic store instantiated for each created registry. If so, I have to admit the terminology gets me tripped up sometimes because a registry can have multiple stores registered on it, so the above sentence reads better to me as "proxify its selectors/actions per registry".

Bake the controls plugin in the namespace stores (it wasn't possible to keep it separate and bake "core/data")

I think this seems sensible given the functionality of controls.

packages/data/src/namespace-store/index.js

packages/data/src/namespace-store/metadata/reducer.js

packages/data/src/store/index.js

packages/data/src/registry.js

nerrad · 2019-03-26T13:17:01Z

With the controls plugin being baked in, I wonder if - in a separate pull after this lands - if we should also bake in some common controls often used. Things like:

apiFetch
select
dispatch
resolveSelect
resolveDispatch (for dispatches that return a value)
subscribe

youknowriad · 2019-03-26T13:19:05Z

@nerrad or maybe a dedicated @wordpress/data-controls

nerrad · 2019-03-26T13:21:25Z

or maybe a dedicated @wordpress/data-controls

Oh ya, even better. That's something that can be done irrespective of this landing. I might give a go at it sometime soon.

aduth

I personally don't have an issue with making controls plugin default behavior. The documentation should be updated accordingly.

packages/data/src/namespace-store/index.js

packages/data/src/plugins/persistence/index.js

packages/data/src/registry.js

packages/data/src/store/index.js

aduth · 2019-03-26T16:17:45Z

packages/data/src/namespace-store/index.js

-	}
+	let resolvers;
+	const actions = mapActions( {
+		...metadataActions,


An initial thought occurred to me that there's risk for conflict between these metadata actions and a store's own. On further consideration, I don't see it likely to occur, or at least it should be known that these "baseline" actions/selectors exist for every store. Which leads me to wonder: How do we document this?

yes, I assume these can't be auto-documented. I'll see if I can add a note in the data module README (where we document registerStore)

yes, I assume these can't be auto-documented.

I'm not so overly concerned about the selectors themselves (though it would be nice), more to just general awareness that this is a behavior to expect, the fact that resolution behavior is included in every store by default.

There's potential side-effects here of consuming code being able to "override" default metadata selectors and actions. I almost wonder if there should be some enforcement here: i.e. checking the incoming actions/selectors and ensuring there are no collisions with the corresponding metadata functions? (or maybe change the order in the constructed array so metadata always overwrites the stores own?).

If we're not confident exposing these selectors and actions yet, we can easily add __internal or __experimental for now, what do you think?

I'm less concerned with exposing them (I think they should be exposed?) and more concerned with potential conflicts.

I do think it'd be better (at least initially) to do this:

// for actions const actions = mapActions( { ...options.actions, ...metadataActions, } ); // and something similar with selectors reversing the order

That way the metadataActions and metadataSelectors take precedence. Of course you may intentionally have them this way so they can be overridden by the incoming store config but if not, I think this is safer.

aduth · 2019-03-26T16:21:14Z

packages/data/src/namespace-store/index.js

+	let selectors = mapSelectors( {
+		...mapValues( metadataSelectors, ( selector ) => ( state, ...args ) => selector( state.metadata, ...args ) ),
+		...mapValues( options.selectors, ( selector ) => {
+			if ( selector.isRegistrySelector ) {


This logic occurs for every selector call? In addition to the overhead of mapSelectors? Should expect a negative performance impact?

I wonder if it's not unreasonable to consider/implement what we have currently as metadata and root as in-fact two separate stores, so that we don't have to override the selector call to reach into state to provide the corrected "root" state.

to your second point, in theory, a store could be just a scalar value which would break this proposal.

to your first point, this only happens when registering the store which I guess is fine. (only run once)

to your first point, this only happens when registering the store which I guess is fine. (only run once)

Subsequent calls would still be subject to going through the ( state, ...args ) => selector( state.root, ...args ) proxying? May be small overhead; curious though given the reality of many thousands of selector calls during typical use.

youknowriad · 2019-03-27T07:57:17Z

Anything that you consider a blocker with this. I'd like to land this to unblock a few other PRs

nerrad

Not a blocker, but I do think (as slim as it is) there's potential for conflict with the resolution selectors (less so actions) (see my earlier comment).

aduth · 2019-03-27T14:01:11Z

Curious why, in Redux DevTools, I see the selectors shown for metadata state. 🤔

aduth

I wasn't able to easily test this against my aforementioned use-case since it requires a combination of this and #14369, but conceptually this seems like a good solution to the general problem.

Still curious about what I'm seeing with Redux DevTools, but preemptively approving.

nerrad · 2019-03-27T14:18:58Z

Curious why, in Redux DevTools, I see the selectors shown for metadata state

Isn't that because resolution state is stored with the selector as the key (i.e no longer need to store indexed by store name)

aduth · 2019-03-27T16:48:37Z

Curious why, in Redux DevTools, I see the selectors shown for metadata state

Isn't that because resolution state is stored with the selector as the key (i.e no longer need to store indexed by store name)

Ah! I think you're right. I still see it as unnatural to have those as direct descendents of a "metadata" key. The reducer is called isResolved, but because it's the default this key isn't shown. Makes me wonder if we ought either rename metadata or have isResolved as a proper key of the metadata state.

Small naming quibble, I'm not overly concerned about it now that it's been explained.

youknowriad · 2019-03-28T07:38:54Z

@aduth It does seem like we could add a metadata.resolution or something like that for this state. If we need more "metadata" for data stores, we can refactor this.

aduth · 2019-03-29T16:08:19Z

packages/data/src/namespace-store/index.js

-				await fulfillment.fulfill( selectorName, ...args );
-				fulfillment.finish( selectorName, args );
+				store.dispatch( metadataActions.startResolution( selectorName, args ) );
+				await fulfillResolver( store, mappedResolvers, selectorName, ...args );


I seem to be running into an issue where this doesn't actually wait for the fulfillment to complete before marking the resolution as finished:

(Note in last three actions, I'd have expected "RECEIVE_ITEMS" to have occurred prior to "FINISH_RESOLUTION")

I tried to consider how to reproduce this outside my branch, but since we wait for most all data to be available before rendering anything, it's hard to recreate even when adding a filter like add_filter( 'block_editor_preload_paths', '__return_empty_array' );. Which has me wondering if it could in-fact be an issue which had already existed prior to this change, and only became evident when performing resolution for data which hadn't been initially requested in the display of the editor (in my case, waiting to resolve getPostType( 'wp_block' ) when saving a reusable block). In my branch, since the resolution completes before the data is available, it results in an error because postType in this assignment resolves to undefined.

I'll try to clean up my local working branch to push later this afternoon, but raising now in case anything comes to mind.

I found the issue. I'll have a pull request shortly.

Edit: #14711

youknowriad self-assigned this Mar 26, 2019

youknowriad requested review from aduth and nerrad March 26, 2019 11:33

Refactor the core/data store to be independent from the registry

9044dc5

youknowriad force-pushed the refactor/core-data-per-store branch from 00b6097 to 9044dc5 Compare March 26, 2019 11:35

nerrad reviewed Mar 26, 2019

View reviewed changes

Fix unit tests

a215c0e

youknowriad marked this pull request as ready for review March 26, 2019 13:11

youknowriad requested a review from gziolo as a code owner March 26, 2019 13:11

youknowriad added 2 commits March 26, 2019 14:43

Override core inline scripts

b440ce8

Fix monkey-patching the stores

43b95fb

aduth reviewed Mar 26, 2019

View reviewed changes

gziolo added [Package] Core data /packages/core-data [Package] Data /packages/data labels Mar 27, 2019

youknowriad added 3 commits March 27, 2019 08:53

Document the empty subscribe function

59d43d2

Destructure metadata

7400e01

forEach instead of map

7e6e3af

nerrad approved these changes Mar 27, 2019

View reviewed changes

aduth approved these changes Mar 27, 2019

View reviewed changes

youknowriad merged commit 1e8d781 into master Mar 28, 2019

youknowriad deleted the refactor/core-data-per-store branch March 28, 2019 07:39

youknowriad mentioned this pull request Mar 28, 2019

Data Module: Support parent/child registries #14369

Merged

youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019

aduth reviewed Mar 29, 2019

View reviewed changes

aduth mentioned this pull request Mar 29, 2019

Data: Return result of middleware chain in resolvers cache middleware #14711

Merged

This was referenced Apr 1, 2019

Bump plugin version to 5.4.0-rc.1 #14735

Merged

Bump plugin version to 5.4.0 #14783

Merged

koke mentioned this pull request Apr 3, 2019

Remove deprecated controls plugin wordpress-mobile/gutenberg-mobile#814

Merged

nerrad mentioned this pull request Apr 4, 2019

Data: Restore dispatch actions returning actions that are a Promise #14830

Merged

8 tasks

aduth mentioned this pull request Apr 15, 2019

If a more recent revision/autosave exists, store its state on editor setup #7945

Merged

4 tasks

nerrad mentioned this pull request May 5, 2019

Packages: Add new @wordpress/data-controls package. #15435

Merged

17 tasks

aduth mentioned this pull request May 23, 2019

Deprecate registry.use() and update internal implementations #11449

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the core/data store to be independent from the registry #14634

Refactor the core/data store to be independent from the registry #14634

youknowriad commented Mar 26, 2019 •

edited

Loading

nerrad left a comment

nerrad commented Mar 26, 2019

youknowriad commented Mar 26, 2019

nerrad commented Mar 26, 2019

aduth left a comment

aduth Mar 26, 2019

youknowriad Mar 26, 2019

aduth Mar 27, 2019

nerrad Mar 27, 2019 •

edited

Loading

youknowriad Mar 27, 2019

nerrad Mar 27, 2019 •

edited

Loading

aduth Mar 26, 2019

aduth Mar 26, 2019

youknowriad Mar 26, 2019

aduth Mar 27, 2019

youknowriad commented Mar 27, 2019

nerrad left a comment

aduth commented Mar 27, 2019

aduth left a comment

nerrad commented Mar 27, 2019

aduth commented Mar 27, 2019 •

edited

Loading

youknowriad commented Mar 28, 2019

aduth Mar 29, 2019 •

edited

Loading

aduth Mar 29, 2019 •

edited

Loading

Refactor the core/data store to be independent from the registry #14634

Refactor the core/data store to be independent from the registry #14634

Conversation

youknowriad commented Mar 26, 2019 • edited Loading

nerrad left a comment

Choose a reason for hiding this comment

nerrad commented Mar 26, 2019

youknowriad commented Mar 26, 2019

nerrad commented Mar 26, 2019

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 27, 2019

nerrad left a comment

Choose a reason for hiding this comment

aduth commented Mar 27, 2019

aduth left a comment

Choose a reason for hiding this comment

nerrad commented Mar 27, 2019

aduth commented Mar 27, 2019 • edited Loading

youknowriad commented Mar 28, 2019

aduth Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

aduth Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

youknowriad commented Mar 26, 2019 •

edited

Loading

nerrad Mar 27, 2019 •

edited

Loading

nerrad Mar 27, 2019 •

edited

Loading

aduth commented Mar 27, 2019 •

edited

Loading

aduth Mar 29, 2019 •

edited

Loading

aduth Mar 29, 2019 •

edited

Loading