Data: Update register behaviors to merge with existing set

[aduth]

This pull request seeks to update the behavior of the data module's registerSelectors, registerActions, and registerResolvers functions to merge into the existing set of selectors, actions, and resolvers, if any exist. Previously it would replace all except those matching keys of the new set). This is intended to facilitate extensibility of singular function handlers, which has been an expected use-case of the data module since its inception (particularly around resolvers fulfillment alternatives).

This is being flagged as a breaking change, since while it was not previously a documented behavior, the new behavior is not compatible with previous expectations. I do not assume that there is much of a likelihood for breakage, and it is not possible to provide a fallback. It is being included in the set of breaking changes slated for a pending @wordpress/[email protected] release.

Included is a new section in the documentation detailing an extensibility example.

In the future, we may consider:

• Refactoring select results to an internally-extensible flow
  • As part of refactoring resolvers as a plugin?
• Offering option to merge controls
  • There is no registerControls function. Should the controls plugin add one?

Testing instructions:

It's not expected that this should have an impact on the application, as to my knowledge there are no instances of multiple calls to register selectors, actions, or resolvers.

Verify still that normal behaviors of each occur as expected (it's unlikely the editor would load at all if they were not functional).

Ensure unit tests pass:

npm run test-unit

[notnownikki]

I'll take a look at this in depth tomorrow, was a little concerned this wouldn't be up for review as quickly as this with the other stuff we have going on, so thank you for this :)

[youknowriad]

Something about this change bothers me. While I conceive that it's convenient to be able to modify existing selectors/actions... I wonder if this is a good thing to allow and not just some kind of shortcut to achieve some extensibility in plugins we won't be able to achieve otherwise.

[aduth]

@youknowriad Taking a look at it from another angle, is it sensible that full replacement is the default behavior? When would this ever be expected or a required use-case? I imagine fully replacing a set of selectors, actions, or resolvers could be destructive if the developer doesn't account for the full set (including those added in the future that they can't anticipate). So part of the agenda here is just choosing a more sensible default, one which happens to support what I'd consider to be the more valid use-case (swapping singular resolvers) while retaining consistency across registrations of the various behaviors.

[notnownikki]

While I'd much rather see the merge happen (we're adapting Gutenberg for a different API and we need to call different endpoints and deal with responses that have a different structure, and doing that in api-fetch middleware isn't nearly as nice as writing a new resolver), I think that if the current behaviour is kept, it needs to be clear in the jsdoc, and possibly change to setX, as you're not registering a bunch of stuff, you're clearing the old and setting the new.

[aduth]

We could also keep the current behavior, but expose the underlying configuration so that if a developer wants to extend a set of e.g. selectors, they could:

registry.registerSelectors( 'foo', {
    ...registry.config.selectors.foo,
    ...myNewSelectors,
} );

[youknowriad]

Actually, my thinking is that neither of the current/proposed behavior are good because both corresponds to monkey-patching functions which is in general not great. I'd be in favor of making those immutable.

That said, I can live with it :).

[notnownikki]

If these are make immutable, doesn't that have the consequence of coupling Gutenberg to the core API quite tightly? I mean, you can adapt things in api-fetch middleware, but you end up intercepting core API calls and translating responses to look like core API responses.

[youknowriad]

@notnownikki Yes it does, and is this worse than monkey patching and assuming the selectors return the same shape of data of the previous selectors?

[notnownikki]

@youknowriad that depends 😄 It's not my decision to make, although my opinion is that we wouldn't want to have that tight coupling, because we already have situations where we want to use it outside of the core API, and for other purposes where we want the majority of WordPress behaviour, but need to subvert bits of it.

The stuff we're doing now can all be done in middleware, I just think we have this awesome data layer that does data resolution, and has selectors that are already being used to apply defaults and change data shape, and it feels like we should be able to say "hey, I use a different API for this bit of functionality, here's the resolver and selector to handle that".

And yes, I'm of the opinion that the contract for data shape should exist with selectors ;)

[youknowriad]

And yes, I'm of the opinion that the contract for data shape should exist with selectors ;)

Maybe, but it does feel wrong to me to replace the selectors, without replacing the actions and the reducer. If I want to replace something, I'd replace everything because selectors + reducer + actions work closely together and unless I know the internals of these, I can't replace just a selector without touching an action and the reducer. While in the other hand, a middleware is a well-defined extensibility API.

[notnownikki]

Maybe, but it does feel wrong to me to replace the selectors, without replacing the actions and the reducer.

🤔

more 🤔

Yes. Agreed 😄 I can't quite think of any situation where we'd want to change a resolver without also changing the selector, except for cases where it's just the path of the API call that's changing, in which case that should be happening in the middleware. And in the case we're currently dealing with, it's coincidence that the action and reducer are the same.

Registering a package of reducer + selectors + resolvers + actions to replace one that's already there? Perhaps in the future if we keep running into situations where we can show that emulating the core API calls and responses is clearly a fragile approach?

(like I say, for how we're modifying things at the moment, the middleware works, I just feel this is worth exploring for cases where things might not be so straightforward)

[aduth]

If these are make immutable, doesn't that have the consequence of coupling Gutenberg to the core API quite tightly? [...]

@notnownikki Yes it does, and is this worse than monkey patching and assuming the selectors return the same shape of data of the previous selectors?

I'd say that yes, it is worse. Ignoring the fact that "monkey patch" is a bit of a loaded phrase, to me the resolution is an incidental behavior and shouldn't be so tightly coupled with the initial implementation. This is what I had intended to convey with the included documentation about a store which tracks temperature information, not caring about from where the data itself be sourced. It's also not fair to say that a resolver replacement would need to know about internal implementation, since the actions are still abstracted as part of the public interface.

Resolvers are the obvious one for replacement here; the others I don't feel as strongly about, except for the mere point of consistency.

[aduth]

I'm getting a sense of quiet disagreement 😄

Taking a step back to approach the broader issue, is resolver substitution something we intend to support? It had been my understanding that we'd wanted to support the option that fulfillment of data requirements could vary depending on the context (considering fulfillment as incidental to store behaviors).

It would be good to clarify if there is at least agreement here before moving forward with how best to accommodate it.

[youknowriad]

I'm getting a sense of quiet disagreement

I disagree with actions/selectors replacement, maybe not resolvers :)

Taking a step back to approach the broader issue, is resolver substitution something we intend to support?

Well for me, instead of achieving this by replacing the resolvers, I don't see why we can't achieve this by just use an api-fetch middleware where the contract is "simpler". request => response (request and response have defined shapes)

---

I'm not strongly against resolvers substitution though :)

[aduth]

I guess where I see this differing from an api-fetch middleware is in my opinion we include default resolvers primarily as a convenience, not that a store need associate its default fulfillment behavior. In that framing, an extreme could be that even the WordPress REST API should be considered a "foreign" resource, no more special than another fulfillment mechanism.

[youknowriad]

Ultimately, you're probably right, I'm a bit concerned about adding this API (or encouraging it) now. It is a bit too soon for this kind of extensibility APIs for me. Because what's behind the scenes is:

• This ties us more to the "resolvers" implementation for fulfilment for the best or the worst.
• This assumes we make it clear how fulfillment should be achieved for each selector. (each selector should have a corresponding action responsible for "fulfilling it")

[aduth]

This assumes we make it clear how fulfillment should be achieved for each selector. (each selector should have a corresponding action responsible for "fulfilling it")

And to be honest, this is a good thing for store design, that a store only be able to operate with actions which allow it to receive new data.

This ties us more to the "resolvers" implementation for fulfilment for the best or the worst.

It's good to raise. On the surface I've felt quite positively toward the notion of selector side effects, but there are still some real remaining concerns surrounding things like e.g. shared dependencies between selectors, that we might want to establish at least some plan / better understanding before further developing APIs around resolvers.

[gziolo]

I'm getting a sense of quiet disagreement

I disagree with actions/selectors replacement, maybe not resolvers :)

I also would be in favor of preventing the possibility to replace the existing actions and selectors. This is public API we agreed on and is used everywhere in the application and can also be used by every plugin. We even expose those items in Gutenberg handbook: https://wordpress.org/gutenberg/handbook/data/.

In theory, it should be fine to allow to add new selectors so one could reuse existing logic in their plugin. Although it isn't essential. To allow the custom action, we would have to allow modifications to the reducers which might break the selectors so I would be very cautious allowing that. Do we have any real-life use cases we are trying to solve here? It would help to identify what is the most important part which needs to be customizable.

Taking a step back to approach the broader issue, is resolver substitution something we intend to support?

Well for me, instead of achieving this by replacing the resolvers, I don't see why we can't achieve this by just use an api-fetch middleware where the contract is "simpler". request => response (request and response have defined shapes)

Well, this was my initial thinking, too. However, I'm not that sure anymore if this is a valid assumption. This would overload api-fetch middleware usage in the cases where it has nothing to do with serving network requests.

[gziolo]

I was thinking how we could update the example provided in the docs in this PR to ensure we can minimize the number of items to override:

registerStore( 'temperature', {
	reducer( state = {}, action ) {
		switch ( action.type ) {
			case 'SET_TEMPERATURE':
				return {
					...state,
					[ action.city ]: action.temperature,
				};
		}

		return state;
	},
	actions: {
		setTemperature,
	},
	selectors: {
		getTemperature: ( state, city ) => state[ city ],
	},
	controls: {
		FETCH( action ) {
			return window.fetch( action.url ).then( ( response ) => response.json() );
		},
	},
	resolvers: {
		* getTemperature( state, city ) {
			const url = 'https://samples.openweathermap.org/data/2.5/weather?q=' + city;
			const json = yield { type: 'FETCH', url };
			yield setTemperature( city, json.main.temp );
		},
	},
} );

it looks like doing a bit of refactoring we can minimize the extensibility point to controls:

controls: {
	FETCH_TEMPERATURE( { city } ) {
		const url = 'https://samples.openweathermap.org/data/2.5/weather?q=' + city;
		return window.fetch( action.url ).
			then( ( response ) => response.json() ).
			then( ( json ) => json.main.temp );
	},
},
resolvers: {
	* getTemperature( state, city ) {
		const result = yield { type: 'FETCH_TEMPERATURE', city };
		yield setTemperature( city, result );
	},
},

which you could customize as follows:

FETCH_TEMPERATURE( { city } ) {
	const url = (
		'https://query.yahooapis.com/v1/public/yql?format=json&env=store%3A%2F%2Fdatatables.org%2Falltableswithkeys&q=' +
		encodeURIComponent( `select * from weather.forecast where woeid in (select woeid from geo.places(1) where text="${ city }")` )
	);
	return window.fetch( action.url ).
		then( ( response ) => response.json() ).
		then( ( json ) => json.query.results.channel.item.condition[ 0 ].temp );
},

In this case it doesn't look any better, it just changes what gets overriden 😃

I bet we should open both controls and resolvers for modification as this gives the flexbility to choose which part should be reimplemented.

[aduth]

I agree that a store's actions and selectors should serve as a public interface, and am not suggesting that we should want to support adding to them, and certainly not removing from them.

The primary use-case which has been raised is around behavior swapping of resolvers. The idea with extending this to selectors and actions is largely for consistency, and I am not strongly committed to it. It is maybe important to highlight that in all of these cases, it's about swapping implementation, not adding to or removing from the public interface of a store.

[gziolo]

It is maybe important to highlight that in all of these cases, it's about swapping implementation, not adding to or removing from the public interface of a store.

Yeah, I would feel more comfortable to allow that in strongly typed language. In this case, it's very fragile as any change to the output shape in a selector may break the whole application 🤷‍♂️

[aduth]

Another point to raise is that this goes against some prior-raised goals of registerStore as the primary API for which to implement a store.

which you could customize as follows:

I'd really not want for the action types to be part of a public contract, which is effectively what would happen here if we expect developers to extend controls.

I'm still inclined that extending resolvers is the right way to go. I wonder then if a good direction would be:

• registerStore acts as an upsert, where the extender calls with their overriding values.
  • Not too much unlike registerBlockType, though in similar fashion open to same debate on whether this is confusing.
• We build-in expectation that developers may attempt to override selectors or actions and, in not wanting to support this, treat these extensions as noop with appropriate warning against attempting to extend / explanation (console.error and/or TypeError).

So effectively we only allow extending resolvers, looking more like:

// Sometime before:
registerStore( 'temperature', { /* ... */ } );

// Sometime later, a plugin extends:
registerStore( 'temperature', {
	resolvers: {
		* getTemperature( state, city ) {
			const url = (
				'https://query.yahooapis.com/v1/public/yql?format=json&env=store%3A%2F%2Fdatatables.org%2Falltableswithkeys&q=' +
				encodeURIComponent( `select * from weather.forecast where woeid in (select woeid from geo.places(1) where text="${ city }")` )
			);
			const json = yield { type: 'FETCH', url };
			yield setTemperature( city, json.query.results.channel.item.condition[ 0 ].temp );
		},
	},
} );

Open question: In the above example, if the original store implementation included other resolvers, would those be respected and only getTemperature replaced? Or is it assumed that if I'm extending resolvers, I'm treating it as though the original implementation for the store is invalid for the current context.

Thoughts?

[youknowriad]

Why coming up with another extensibility API (calling multiple times) where we can just add a filter on the resolvers?

[gziolo]

Why coming up with another extensibility API (calling multiple times) where we can just add a filter on the resolvers?

@aduth, I had a very similar thought when I saw your comment. If we use a filter, it would align with what we already offer for both blocks and plugins:

• https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/registration.js#L106
• https://github.com/WordPress/gutenberg/blob/master/packages/plugins/src/api/index.js#L55

This would be much easier for developers to who are familiar with extending blocks and gives them the flexibility to decide whether they want to add new resolvers or update existing ones. In fact, they might even want to remove one of the resolvers if they think it is obsolete but still gets triggered.

[gziolo]

@youknowriad and @aduth - this isn't something on the roadmap for Phase 2 but I would love to hear your thought whether you plan to invest more time into this proposal? If yes, it needs to be refreshed. I'm marking as Stale to make triaging easier in the future.

[aduth]

I think this can serve useful as background discussion for a revived attempt in the future, but I won't plan to revisit this branch in its current form.