I like it 👍

Yes, much nicer. I think this should be the only public API available. We could turn registerReducer, registerActions and registerSelectors into the implementation detail.

Unless we want to allow other plugins to make modifications to not their own stores.

I like it a bit less, but not a blocker :) I can see the advantage of factorizing this kind of logic we could need in several places.

The alternative being:

let lastBlocks;
subscribe( () => {
	const blocks = select( 'core/editor' ).getBlocks();
	if ( blocks !== lastBlocks ) {
		// ... Blocks are known to have changed.
	}
	lastBlocks = blocks;
} );

Aside from being verbose and clumsy, there's another issue here: The selector is called on every single state change to every single registered store. For expensive queries (even memoized), this would be quite resource intensive.

https://github.com/WordPress/gutenberg/blob/4ea8dd0ac5d38788adad8b093738eb54fa32b8bd/edit-post/store/effects.js#L42-L48 :)

Right, but in your example the subscribe logic is run when core/viewport state changes, which is not so great.

Minor not a blocker just a thought: In redux subscribes always receives a listener and there is no "filtering" possibility. In my opinion, I think we should also do the same, subscribe always receives just a listener, so we are consistent with redux. I understand filtering a specific change will be very a common case, so to address we can use: compose( selectorListner(selector), reducerListner( reducer ), subscribe ). This makes clear that some code is always executed for each change, and our functions selectorListner and reducerListner are just helpers to filter out change. Other helpers can be created in the future.

@jorgefilipecosta This is interesting. At its most generic, it really only requires a "has value changed" higher-order function to encapsulate the behavior from the original alternative. Something like:

subscribe( ifChanged( () => (
	select( 'core/viewport' ).isViewportMatch( '< medium' )
) )( () => {
	const isSmall = isViewportMatch( '< medium' );

	// Collapse sidebar when viewport shrinks.
	if ( isSmall ) {
		dispatch( 'core/edit-post' ).closeGeneralSidebar();
	}
} ) );

Then it becomes a question of: Do we expect this to be such a common pattern that it's still worth building conveniences into the API itself? The above snippet is generic, and not terribly verbose or clumsy, but still more verbose than the changes that had been proposed here. Generalism can be an ideal, but it shouldn't deter us from creating conveniences for common patterns.

It's a discussion worth having though:

• What other subscribe helpers could we imagine?
• What sorts of things would we expect people to use selector subscriptions for?

To the latter question, I'm finding that this could satisfy any need for a side effect pattern in the data module. In fact, taken to its extreme, we might be able to replace all of our own internal side effects with this approach, representing the side effects as a result of a change in state. Can all of the current and foreseen side effects be stated this way, or is access to the raw actions, even those not resulting in a change, necessary? It'd require some investigation, but I'm inclined to say that we could and should try to model side effects on changes in state. This might help remedy some of the disorganization that has resulted in the various effects.js files. At a higher-level, it might even be seen as the data equivalent of React's visual declarative UI, where "props" in this case are the selector result(s).

As a testament to the clumsiness of #5206 (comment), this example code snippet inadvertently introduces an infinite loop 😏 (Dispatch causes subscribe to re-invoke before lastBlocks is assigned)

Thank you for thoughts and analysing this, it is good that we will be able to discuss this specifc part outside of the scope of the rest of the changes 👍

I believe this is a new behavior, how was this handled before? The separation between the mobile sidebar and the desktop sidebar?

It was previously effected in edit-post/store/reducer.js's handling of the UPDATE_MOBILE_STATE action triggered by enhanceWithBrowserSize.

missing element, data and components here ;)

Yes, it should be removed with #4777, but my comment was missed.

Yes, this one is cool 👍

It seems like when and whenWith should be extracted to their own file so we could have only public APIs in this file. Can we also add simple unit tests for both functions?

Create our own FP utility library? 😄

I'm fine about pulling ramda and recompose in 😄

Since 84c64c8 was removed in rebase, this is no longer relevant.

Minor: when expect fails unsubscribe methods won't be executed. It shouldn't have impact on other tests though. One way to solve it is to put all unsubscribe methods in an array and iterate over it using afterEach.

let unsubscribes = [];
afterEach( () => {
    unsubscribes.forEach( ( unsubscribe ) => unsubscribe() );
    unsubscribes = [];
} );
it( '...', () => {
    unsubscribes.push( subscribe( 'one', oneCallback ) );
} );

or something like this.

Yeah, that's true. At the same time though, if a test fails, all bets are off. The only issue is if it impacted future failures that caused needless debugging. I'll see about adding lifecycle.

Exactly, it might fail not related tests in the future. It’s a minor thing and really hard to implement in a clean way 🙁

Since 84c64c8 was removed in rebase, this is no longer relevant. That said, before dropping, I did find it was pretty simple to implement a helper subscribe wrapper:

const unsubscribes = [];
afterEach( () => {
	let unsubscribe;
	while ( ( unsubscribe = unsubscribes.shift() ) ) {
		unsubscribe();
	}
} );

function subscribeWithUnsubscribe( ...args ) {
	unsubscribes.push( subscribe( ...args ) );
}

This one might be confusing because it behaves differently than when it contains only one param. Should we create 2nd HOC component instead? Example:

MyComponent = withViewportProps( { isMobile: '< small' } )( MyComponent );

This one might be confusing because it behaves differently than when it contains only one param. Should we create 2nd HOC component instead?

I'd totally agree from a consumer's perspective these make sense as two separate components. My only worry is that from the component author's perspective, it's a fair amount of work if we want all "if" HOC to also have a complementary "with".

I might imagine we could leverage some composition here though:

ifViewportMatches = compose( [
	withViewportMatch( '< small' ),
	ifPropsMatch( { isViewportMatch: true }, { omitProps: true } ),
] );

That’s interesting idea to compose two HOCs where both of them might be useful by their own, too.

Split to separate higher-order components withViewportMatch and ifViewportMatches in fe100d4.

Minor: I had to spend some time to understand what's happening here. It might be only me, so feel free to ignore. It might be more code to express it differently, but maybe it would pay off. Example:

const key = query.split( ' ' ).length === 1 ? `>= ${ query }` : query;

I think it will work with the existing tests.

Minor: I had to spend some time to understand what's happening here.

Yeah, it's one of my favorite "clever" codes that I still find myself leaning on. 😄 I might argue it's a valuable pattern that we can introduce developers to. For example, I've also found value in using it to format a date as MM-DD-YYYY (where the source value of MM and DD can be one or two digits):

[ d.getMonth() + 1, d.getDate() ].map( ( n ) => ( '0' + n ).slice( -2 ) ).concat( d.getFullYear() ).join( '-' )
// "02-23-2018"

Maybe some clarifying comments explaining the behavior?

const lastTwo = items => items.slice( -2 );
cons key = lastTwo( [ '>=', ...query.split( ' ' ) ] ).join( ' ' );

For me personally, this would help :)

For me personally, this would help :)

https://lodash.com/docs/4.17.5#takeRight 😄

Added an inline comment and used Lodash's _.takeRight in rebased 217021a.

Should it be getWrapperDisplayName( WrappedComponent, 'ifCondition' );?

We omit prefixes in other places, but we might change this if this is confusing.

We omit prefixes in other places, but we might change this if this is confusing.

I'm thinking particularly with #5206 (comment) in mind, we might want to do this, since we could have two higher-order components with similar names distinguished on prefix, e.g. withViewportMatch, ifViewportMatches.

Also to the point of #5206 (comment), I'm starting to think maybe we could use a new higher-order component utility in place of the current getWrapperDisplayName to allow for easier HOC composition.

Example:

export default createHigherOrderComponent(
	'ifViewportMatches',
	( query ) => compose( [
		withViewportMatch( { isViewportMatch: query } ),
		ifCondition( ( props ) => props.isViewportMatch ),
	] )
);

@aduth, this is actually a great idea even for the regular HOCs. See:

const ifCondition = ( predicate ) => ( WrappedComponent ) => {
	const EnhancedComponent = ( props ) => {
		if ( ! predicate( props ) ) {
			return null;
		}

		return <WrappedComponent { ...props } />;
	};

	EnhancedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'ifCondition' );

	return EnhancedComponent;
};

vs.

const ifCondition = createHigherOrderComponent(
	'ifCondition',
	( predicate ) => ( WrappedComponent ) => {
		return ( props ) => {
			if ( ! predicate( props ) ) {
				return null;
			}

			return <WrappedComponent { ...props } />;
		};
	},
);

I'm voting to refactor all HOCs to use the helper you proposed and update all prefixes to match what has been discussed here.

Implementation wise it might be non-trivial task because some HOCs take additional params and require a higher-order function to be involved :)

Implementation wise it might be non-trivial task because some HOCs take additional params and require a higher-order function to be involved :)

Yeah, I was thinking about this too. One option is to test whether the argument passed is a class extending Component, but this would mean that all higher-order components would have to be implemented as the full-on class form. Maybe not a big deal to impose this?

can we do something similar to what gutenberg/edit-post/store/constants.js does? Use the scss variable loader '!!sass-variables-loader!../assets/stylesheets/_breakpoints.scss';

can we do something similar to what gutenberg/edit-post/store/constants.js does? Use the scss variable loader '!!sass-variables-loader!../assets/stylesheets/_breakpoints.scss';

My main issue with this is that the breakpoints are defined in an edit-post stylesheet. Ideally those common SCSS variables don't remain there. The main issue we solve with the variable loader is avoiding repetition, but one of the goals with viewport module is at least to consolidate all viewport-related logic. So hopefully at most we'd have at most two sets to maintain: one in SCSS and one in JS.

Yes it's trade that makes sense to make, and as it is just this cases, it is not hard to maintain 👍

Would it make any difference in case ifViewportMatches( 'medium' ) would be composed as the first item here? All HOCs would have to be applied when the source file loads, but it seems like withSelect and withDispatch wouldn't have to be rendered in case the viewport doesn't match. It might only make any difference on the initial render if I process the application flow properly.

Would it make any difference in case ifViewportMatches( 'medium' ) would be composed as the first item here?

I'll test it, but yes, this seems like it could potentially serve as a (small) optimization.

It looks really nice. Thanks for taking my feedback into account 👍

So much nicer 💯

Just noting that it has too many dependencies to move it to @wordpress/packages.

I'm still not sure whether we shoud pick the signature closer to what you proposed before:

MyComponent = withViewportMatch( 'isMobile', '< small' )( MyComponent );

or

MyComponent = withViewportMatch( { isMobile: '< small' } )( MyComponent );

The latter seems to be more flexible, but we don't have any use case in the code that validates we need that capability.

I'm still not sure whether we shoud pick the signature closer to what you proposed before:

In my initial attempt at splitting the components, I'd gone with the first of the two, but upon some reflection I think the object form is better:

• More familiar in other usage of higher-order components which map to props as an object (e.g. withSelect)
• Allows for multiple viewport props to be provided (handling multiple cases of viewport size)

Yes, probably is the best to keep it as it is.

This is amazing, how you composed 2 existing HOCs to create another one 🚀