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

Introduce the concept of registry selectors #13662

Merged
merged 2 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@youknowriad
Copy link
Contributor

youknowriad commented Feb 5, 2019

In some situations, you want to build selectors that target multiple stores at the same time. Until now we were relying on the global select function but the issue is that it only targets the default registry. If we have a separate provider, this might not work as expected. In this PR, I'm introducing a createRegistrySelector helper used to mark a selector a cross-stores selector and providing a registry object.

const myRegistrySelector = createRegistrySelector( registry => (state, ...args) => {
 // Do something with registry
});

Testing instructions

  • Ensure that when pasting a link to embed, the "loading" state is still shown properly.

This is a requirement for #13088

@youknowriad youknowriad requested review from aduth , gziolo and nerrad as code owners Feb 5, 2019

@youknowriad youknowriad self-assigned this Feb 5, 2019

@nerrad

nerrad approved these changes Feb 5, 2019

Copy link
Contributor

nerrad left a comment

Looks good except for the nitpick comment I had (approved regardless).

I'm curious, what scenarios would there be where multiple registry providers would be registered in the same session? I'm guessing this is more for plugins that might register their own registry provider (rather than the default one) and might still need to select from the default registry?

@@ -19,7 +19,7 @@ import createResolversCacheMiddleware from './resolvers-cache-middleware';
*
* @param {string} key Identifying string used for namespace and redex dev tools.
* @param {Object} options Contains reducer, actions, selectors, and resolvers.
* @param {Object} registry Temporary registry reference, required for namespace updates.
* @param {Object} registry registry reference.

This comment has been minimized.

@nerrad

nerrad Feb 5, 2019

Contributor

Nitpick:

Suggested change Beta
* @param {Object} registry registry reference.
* @param {Object} registry Registry reference.
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 5, 2019

I'm curious, what scenarios would there be where multiple registry providers would be registered in the same session?

Actually, we have a need for it inside Gutenberg itself. Right now the implementation of the reusable blocks editor is not ideal, we're forced to include their content in the root blocks reducer. Ideally the reusable block's editor is just an embedded editor inside Gutenberg which means another registry inside the Gutenberg registry.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

So code calling a global selector will still need to access whatever registry it is calling the selector from right (as illustrated by the test)?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 5, 2019

yes, that's the idea.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

So is the primary benefit here so that you can import the same selectors for registration with different registries and avoid cross pollution (i.e those cross store selectors in one registry will be pulling from the state specific to the store in that registry)?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 5, 2019

This more a "fix" than an "improvement". Registries are supposed to be separate data holders and selectors pure functions to be called once the state of the registry (multiple store in the registry) changes.

If we do not ensure that we're calling a selector in a store from the same registry, we're not certain that this selector is being called properly once the state of the said store changes.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

Ahh gotcha, so this is more a tightening up of things to prevent possible future bugs as further improvements are made. All clear now 👍

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

So when this lands, will we want to update controls.js in the @wordpress/core-data package so that instead of using the select global from the default registry, it's using createRegistrySelector?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 5, 2019

So when this lands, will we want to update controls.js in the @wordpress/core-data package so that instead of using the select global from the default registry, it's using createRegistrySelector?

This is a good question and yes we need something like createRegistrySelector for controls as well. Controls should have access to the registry to avoid calling the globals.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 5, 2019

In that vein then, we'll probably need an equivalent creator for dispatch as well.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019

@youknowriad youknowriad merged commit f0bb097 into master Feb 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/registry-selectors branch Feb 6, 2019

@aduth
Copy link
Member

aduth left a comment

I'd wanted to have had a chance to look at it, but in reflection it's about exactly what I'd hoped to have seen for an interface. Nice 👍

I still think it'll pose a challenge for something like #13177 in following dependencies, where the only other alternative I could have imagined in my mind was one where the selector made more explicit the stores/selectors from which it needed to select dependant data.

const createStateSelector = ( selector ) => function runSelector() {
function mapSelectors( selectors, store, registry ) {
const createStateSelector = ( registeredSelector ) => function runSelector() {
const selector = registeredSelector.isRegistrySelector ? registeredSelector( registry ) : registeredSelector;

This comment has been minimized.

@aduth

aduth Feb 6, 2019

Member

While the logic here is fairly trivial, runSelector is our hottest path in the application.

Is it possible to assign this once when mapSelectors is first called? The registry should stay constant, correct?

This comment has been minimized.

@youknowriad

youknowriad Feb 6, 2019

Author Contributor

Good catch, I originally wanted to implement this feature regardless of the store implementation (outside namespace-store) but it means the computation is done on each select so I moved it here to solve the issue but it seems like I didn't :P I just thought I did. I'll follow-up

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 6, 2019

In that vein then, we'll probably need an equivalent creator for dispatch as well.

Isn't this also a blocker for #13088 , @youknowriad ?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 6, 2019

Isn't this also a blocker for #13088 , @youknowriad ?

Probably, it means we need the registry-aware controls, I'll see what I can do.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 11, 2019

In reflecting on this, I'm a bit afraid of the implications of what we've started to introduce, particularly with respect to:

  • The ability to derive dependencies in a manner like that explored in #13177
  • More importantly, a lack of necessary restrictions by making the entire registry available to a selector (e.g. the newly-introduced ability to dispatch actions from a selector)

The alternatives I see are respectively limited and more extreme, where the more extreme option is less usable but more accommodating to options for statically analyzing dependencies.

The first being: Don't pass the registry, just pass select:

export const isRequestingEmbedPreview = createDerivedSelector( ( select ) => ( state, url ) => {
	return select( 'core/data' ).isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
} );

The "extreme" being: Force the developer to define the stores upon which they depend:

export const isRequestingEmbedPreview = createDerivedSelector(
	// (Maybe to avoid ugliness around figuring out good variable names, we
	// still pass this argument as `select`, optionally limited to only the
	// stores which are provided in the second argument)
	( coreData ) => ( state, url ) => {
		return coreData.isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
	},
	[ 'core/data' ]
 );

While not totally relevant, I came to this worry as a consequence of considering the idea of cross-state history discussed elsewhere ([1] [2]), where an idea in my head had started to form around history being defined as its own store, with its own state, etc. I'd thought of it in considering how we deal with state which responds to other store's state, since I'd thought maybe of the history store's state being updated when another store changes, and whether that implies two separate store changes, and two cascading withSelect subscriber updates.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 11, 2019

  • (e.g. the newly-introduced ability to dispatch actions from a selector)

whoah, I didn't even think of that! That could admittedly be a potential problem (dispatching an action that in turn calls the selector!).

The "extreme" being: Force the developer to define the stores upon which they depend

While the described api is a bit more verbose, I like the explicit description of what stores are dependencies. At a minimum, it's probably good to only expose the select for use here (so the first example).

I wonder in relation to the undo/redo discussion if registered stores should be given a unique id on registration and that maybe could assist with cross store history. This might introduce the need for some sort of registry state that keeps track of all the registered stores and their ids? Some potential uses for the id:

  • internal tracking of which store actions can be used for replay (i.e. track no only the action but also the store id).
registryStoreActivity = {
	[ storeAId ]: [
		UPDATE: { ...args },
		SAVE: { ...args },
	],
	[ storeBId ]: [
		SWITCH: { ...args },
		TYPING: { ...args },
	],
	all: [
   		[ storeAId, 'UPDATE', {...args} ],
		[ storeBId, 'SWITCH', { ...args } ],
		[ storeAId, 'SAVE', { ...args } ],
		[ storeBId, 'TYPING', { ...args } ],
	],
};

In the above example, individual store activity is tracked so replay can just be done on the store level. There's also an "all" key that tracks actions across all stores (so replay can be done on the global level).

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 12, 2019

export const isRequestingEmbedPreview = createDerivedSelector( ( select ) => ( state, url ) => {
	return select( 'core/data' ).isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
} );

I like this proposal personally, because it's very similar to withSelect

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2019

I like this proposal personally, because it's very similar to withSelect

In fact I was very inclined to call it withSelect 😄 The name as proposed should be considered working; I'm not in love with it.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2019

At a minimum, it's probably good to only expose the select for use here (so the first example).

Yes, this seems most actionable / non-controversial.

I wonder in relation to the undo/redo discussion if registered stores should be given a unique id on registration and that maybe could assist with cross store history.

I'd thought similar on a registry level. I've not yet fleshed out the implementation, but I'm inclined to see if I can avoid needing something like this. Ideally the store name, within a given registry, is sufficient. The idea of having history as a store itself can help here, since it would be unique per registry.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Feb 12, 2019

In fact I was very inclined to call it withSelect 😄 The name as proposed should be considered working; I'm not in love with it.

I personally am okay with select as the name. Effectively, it describes what it's doing. If you want to get very specific though maybe storeSelect?

Also related to this convo, should similar treatment be given to the createRegistryControl function? So it could be something like:

export default {
	SELECT: createRegistryControl(
		( { select } ) => ( { reducerKey, selectorName, args } ) => {
			return select( reducerKey )[ selectorName ]( ...args );
		}
	)
}

So essentially, for controls the registry exposes a subset of registry functions as an object for callbacks to receive.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 18, 2019

In reflecting on this, I'm a bit afraid of the implications of what we've started to introduce, particularly with respect to:

Revisions per feedback continued at #13889

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment