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

Data Module: Support parent/child registries #14369

Merged
merged 2 commits into from Mar 28, 2019
Merged

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Mar 11, 2019

Extracted from #14367

The idea of this PR is the possibility to override existing data registries by changing one or multiple stores in it. For example, this is useful when you want to keep using the global stores for viewport, ... but you want to have a separate store for your own block editor instance.

See #14367 for an exemple of use-case.

@youknowriad youknowriad added this to the 5.3 (Gutenberg) milestone Mar 11, 2019
@youknowriad youknowriad self-assigned this Mar 11, 2019
@youknowriad youknowriad requested review from aduth and nerrad as code owners Mar 11, 2019
Copy link
Contributor

nerrad left a comment

So reviewing this and the linked use-case, can you tell me if I'm interpreting things correctly? The purpose of this pull is so that there is now the ability to register a new data registry with another registry as the "parent". Any subsequent stores registering on the new registry that exist on the "parent" will get overridden.

If correct, then I think it'd be useful to see the following:

  • A unit-test that demonstrates only the overridden store is modified (i.e. have another store on the parent that is not overridden in the subRegistry registered stores).

Also, I wonder if we'll hit a potential "gotcha" in the future in the case where overridden actions/selectors/reducers for a given parent do not have the same signatures as the parent. In other words what if the parent has the following selectors:

  • getFoo( state, someVariable )
  • getBar( state, someVarA, someVarB )
  • getFooBar( state )

Then let's say a new registry is created and it overrides the parent store, but it's selectors have the following signature:

  • getFoo( state )
  • getBar( state, someVarA )

Will it matter that the overridden store has no similarity to the parent?

I'm assuming no because the consuming code should be aware of what registry it's using, but for thoroughness I thought it worth asking.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Mar 12, 2019

A unit-test that demonstrates only the overridden store is modified (i.e. have another store on the parent that is not overridden in the subRegistry registered stores).

I have a unit test showing that the child action/selector are called but not the parent, which is the same thing for me as stores can have different implementations.

Will it matter that the overridden store has no similarity to the parent?

You're right, It doesn't matter I think, I don't think currently see any potential usage for this pattern but I don't see it as problematic. we replace the entire store with its selectors, actions...

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@youknowriad youknowriad requested review from nerrad and WordPress/gutenberg-core Mar 19, 2019
@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Mar 19, 2019

So looking more closely at the implementation example given, the behaviour of this registration is it only affects things interacting with the registry in the new branch right? For example anything in the reusable block component tree will be using the overridden core/block-editor registry separate from what was globally registered higher in the tree? I'm still trying to grasp the implications of this.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Mar 19, 2019

@nerrad yes, that's correct.

@nerrad
nerrad approved these changes Mar 19, 2019
Copy link
Contributor

nerrad left a comment

Thanks for clarifying things for me. Makes sense and looks useful.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Mar 19, 2019

Clarifying for myself: It's a bit like extending a registry, correct? Where a new registry is created, and unless a store is overridden, it falls back to the implementation from its immediate parent? The main advantage over something like a clone function considered long ago in #7453 would be that the parent is still "live" (i.e. can have more stores registered later, or state changes applied) with the proposed implementation here? I can see how that would be desirable.

@@ -171,5 +180,9 @@ export function createRegistry( storeConfigs = {} ) {
...storeConfigs,
} ).map( ( [ name, config ] ) => registry.registerStore( name, config ) );

if ( parent ) {
parent.subscribe( globalListener );

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

It's nice how simply this works 👍

It may prove to cause #13177 to be more difficult to implement, if we choose to pursue it again in the future (specifically, the change in signature to subscribe( listener[, reducerKeys ] ).

@@ -171,5 +180,9 @@ export function createRegistry( storeConfigs = {} ) {
...storeConfigs,

This comment has been minimized.

Copy link
@aduth

aduth Mar 19, 2019

Member

The line prior: We register core/data by default. The store tracks resolutions, which feels like something we should want to be considered globally, maybe represented by only registering it by default for the root registry. What do you think?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 19, 2019

Author Contributor

yes, core/data is proving to be more and more challenging :). to be honest I feel we have two options:

  • Have a single global core/data but the question of how to differentiate between stores with similar keys and different registries will be asked.
  • Instead of having core/data being a store, having more like a reducer embedded in all stores. There's some backward compatibility concerns with this approach but what I like about it is that it makes the stores even more independent from the registries (related to this comment #14367 (comment))

This comment has been minimized.

Copy link
@nerrad

nerrad Mar 19, 2019

Contributor

Instead of having core/data being a store, having more like a reducer embedded in all stores. There's some backward compatibility concerns with this approach but what I like about it is that it makes the stores even more independent from the registries (related to this comment #14367 (comment))

This approach sounds interesting to me. How would the actions on core/data be exposed (or would there be a special api for the resolution actions/selectors?)

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 19, 2019

Author Contributor

How would the actions on core/data be exposed

We could keep them exposed as is (like a virtual store for backwards compatibilty) or have separate actions/selectors for each store (we could even do this today as a shortcut).

If this is something you're interested in exploring, feel free to do so :)

This comment has been minimized.

Copy link
@nerrad

nerrad Mar 19, 2019

Contributor

If this is something you're interested in exploring, feel free to do so :)

I'd love to. Unfortunately my contributions right now are pretty limited to grunt work/feedback/reviews as my client workload has increased. Still I might manage to carve out some personal time to experiment.

This comment has been minimized.

Copy link
@aduth

aduth Mar 22, 2019

Member

The line prior: We register core/data by default. The store tracks resolutions, which feels like something we should want to be considered globally, maybe represented by only registering it by default for the root registry. What do you think?

Noting that in a variant of #14367 with Reusable Block implemented with an embedded <EditorProvider />, this proved to be quite problematic, where saving would not complete because a resolveSelect would never resolve due to the fact that resolution conditions were considered on its own copy of core/data, despite the fact that resolution had occurred on the inherited core store (from the parent, with its own different core/data).

I guess at the very least, hasFinishedResolution would need to be aware of whether the storeKey it receives is one defined in the current registry, or which would be inherited from its parent.

  • Have a single global core/data but the question of how to differentiate between stores with similar keys and different registries will be asked.

Depending on whether it's possible and without needing to impact the external API, I wonder if it's a matter of creating some unique identifier per resolver and tracking that as part of resolution status in the "global" core/data. I guess it'd still be challenging in that if a registry was using an inherited store, considering resolution status would have to check its parent registry instead.

This comment has been minimized.

Copy link
@nerrad

nerrad Mar 22, 2019

Contributor

Just throwing out, I wonder if resolution state should be enhanced on registered store state as opposed to global state? That way its specific to the stores?

This comment has been minimized.

Copy link
@aduth

aduth Mar 25, 2019

Member

Just throwing out, I wonder if resolution state should be enhanced on registered store state as opposed to global state? That way its specific to the stores?

Yeah, this sounds similar to what @youknowriad suggested with the embedded reducer. On the note of backwards-compatibility, I don't think it's an issue if the idea is that we still offered selectors from core/data to check resolved status. The problem there is how we'd reimplement those selectors to check the states of the individual stores.

I could see it being a compatibility issue if we're saying that we'd extend the stores with additional selectors like select( 'core/editor' ).hasFinishedResolution, but it wasn't clear to me that's what was proposed, or what's desired.

This comment has been minimized.

Copy link
@aduth

aduth Mar 25, 2019

Member

The line prior: We register core/data by default. The store tracks resolutions, which feels like something we should want to be considered globally, maybe represented by only registering it by default for the root registry. What do you think?

Noting that in a variant of #14367 with Reusable Block implemented with an embedded <EditorProvider />, this proved to be quite problematic, where saving would not complete because a resolveSelect would never resolve due to the fact that resolution conditions were considered on its own copy of core/data, despite the fact that resolution had occurred on the inherited core store (from the parent, with its own different core/data).

Trying to rephrase this, because in re-reading it, I don't think it's very clear:

In the embedded editor, when a save occurs, the implementation of the action will cause a selector call to select( 'core' ).getPostType. Since the core store is not explicitly registered in the child registry, it will be called against that of its parent. The resolver still takes effect, but it checks resolution status against the parent's core/data store. Since getPostType would have been resolved much earlier in the parent store, the resolver never triggers its fetch. From the perspective of the child store, which has its own instance of core/data, the resolution still has not occurred, so it will forever become stuck.

Locally, I resolved this with the following:

diff --git a/packages/data/src/registry.js b/packages/data/src/registry.js
index 9f8268360..2f4bb41bd 100644
--- a/packages/data/src/registry.js
+++ b/packages/data/src/registry.js
@@ -175,10 +175,16 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
 		return registry;
 	}
 
-	Object.entries( {
-		'core/data': dataStore,
-		...storeConfigs,
-	} ).map( ( [ name, config ] ) => registry.registerStore( name, config ) );
+	if ( parent === null ) {
+		storeConfigs = {
+			'core/data': dataStore,
+			...storeConfigs,
+		};
+	}
+
+	Object.entries( storeConfigs ).forEach( ( [ name, config ] ) => {
+		registry.registerStore( name, config );
+	} );
 
 	if ( parent ) {
 		parent.subscribe( globalListener );

But to @youknowriad's point above, the problem becomes that, from core/data, we distinguish on store name and selector, selector args, but notably not registry, so there's a high potential for conflict if two stores in separate registries each have their own need to resolve.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 28, 2019

Author Contributor

Now that #14634 is merged, it seems that we can move forward here

@youknowriad youknowriad force-pushed the add/parent-registry-support branch from b7fd124 to a6b33bd Mar 28, 2019
@youknowriad youknowriad merged commit c7f846a into master Mar 28, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the add/parent-registry-support branch Mar 28, 2019
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.