Add registry param to withDispatch component

[gziolo]

Description

In the majority of cases, it will be sufficient to use only two first params passed to mapDispatchToProps. However, there might be some very advanced use cases where using select function might be used as a tool to optimize the performance of your component. It is useful when you need to fetch some dynamic data from the store at the time when the event is fired, but at the same time, you never use it to render your component. In such scenario, you can avoid using the withSelect higher order component to compute such prop, which might lead to unnecessary re-renders of you component caused by its frequent value change.

TODO

• Update docs
• Update code occurences which pass selectors as prop

How has this been tested?

Unit tests: npm run test

1. Open an existing post (can be the demo).
2. Select all blocks (cmd+a twice) and copy them using cmd+c.
3. Paste them in the new empty post with cmd+v.
4. Select all blocks again and cut them using cmd+x.
5. Paste them in the new empty post with cmd+v.

Types of changes

Refactoring.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[aduth]

Like I'd mentioned at #11495 (comment), I'm not really a fan of the idea, because inevitably I think a developer would expect to be able to...

it( 'provides selected data as a prop?', () => {
	registry.registerStore( 'counter', {
		reducer( state = 0, action ) {
			if ( action.type === 'increment' ) {
				return state + 1;
			}
			return state;
		},
		actions: {
			increment: () => ( { type: 'increment' } ),
		},
		selectors: {
			getCount: ( state ) => state,
		},
	} );

	const Component = withDispatch( ( _dispatch, ownProps, _select ) => {
		const count = _select( 'counter' ).getCount();

		return {
			count,
			increment: _dispatch( 'counter' ).increment,
		};
	} )( ( props ) => (
		<button onClick={ props.increment }>
			{ props.count }
		</button>
	) );

	const testRenderer = TestRenderer.create(
		<RegistryProvider value={ registry }>
			<Component />
		</RegistryProvider>
	);

	registry.dispatch( 'counter' ).increment();

	expect( testRenderer.root.findByType( 'button' ).props.children ).toBe( 1 );
} );

[gziolo]

Haha, you caught me. This is what I was planning to do:

diff --git a/packages/editor/src/components/copy-handler/index.js b/packages/editor/src/components/copy-handler/index.js
index e74cde91b..fb66758c3 100644
--- a/packages/editor/src/components/copy-handler/index.js
+++ b/packages/editor/src/components/copy-handler/index.js
@@ -26,18 +26,7 @@ class CopyHandler extends Component {
 	}
 
 	onCopy( event ) {
-		const { hasMultiSelection, selectedBlockClientIds, getBlocksByClientId } = this.props;
-
-		if ( selectedBlockClientIds.length === 0 ) {
-			return;
-		}
-
-		// Let native copy behaviour take over in input fields.
-		if ( ! hasMultiSelection && documentHasSelection() ) {
-			return;
-		}
-
-		const serialized = serialize( getBlocksByClientId( selectedBlockClientIds ) );
+		const serialized = serialize( this.props.getSelectedBlocks() );
 
 		event.clipboardData.setData( 'text/plain', serialized );
 		event.clipboardData.setData( 'text/html', serialized );
@@ -65,7 +54,6 @@ export default compose( [
 		const {
 			getMultiSelectedBlockClientIds,
 			getSelectedBlockClientId,
-			getBlocksByClientId,
 			hasMultiSelection,
 		} = select( 'core/editor' );
 
@@ -75,13 +63,28 @@ export default compose( [
 		return {
 			hasMultiSelection: hasMultiSelection(),
 			selectedBlockClientIds,
+		};
+	} ),
+	withDispatch( ( dispatch, ownProps, select ) => {
+		const { getBlocksByClientId } = select( 'core/editor' );
+		const { removeBlocks } = dispatch( 'core/editor' );
+
+		return {
+			getSelectedBlocks: function() {
+				const { hasMultiSelection, selectedBlockClientIds } = ownProps;
+
+				if ( selectedBlockClientIds.length === 0 ) {
+					return;
+				}
+
+				// Let native copy behaviour take over in input fields.
+				if ( ! hasMultiSelection && documentHasSelection() ) {
+					return;
+				}
 
-			// We only care about this value when the copy is performed
-			// We call it dynamically in the event handler to avoid unnecessary re-renders.
-			getBlocksByClientId,
+				return serialize( getBlocksByClientId( selectedBlockClientIds ) );
+			},
+			onRemove: removeBlocks,
 		};
 	} ),
-	withDispatch( ( dispatch ) => ( {
-		onRemove: dispatch( 'core/editor' ).removeBlocks,
-	} ) ),
 ] )( CopyHandler );

[aduth]

In considering this further, and in attention of a common pattern of components receiving props for the sole purpose of shuffling them through to the dispatch callback in the limited cases at which they are relevant (example), I'll retract my hesitation.

If we can assure that the properties of the return value of mapDispatchToProps are functions, we can have a reasonable guarantee that those functions, once called, can receive the latest state, due to the behavior of withDispatch in re-calling mapDispatchToProps at the time the function is called.

[gziolo]

If we can assure that the properties of the return value of mapDispatchToProps are functions, we can have a reasonable guarantee that those functions, once called, can receive the latest state, due to the behavior of withDispatch in re-calling mapDispatchToProps at the time the function is called.

It has been addressed with 99d290b. I added unit test which ensures that function will throw an error when something other than function will throw an error.

[gziolo]

https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/components/header/index.js#L93

So this makes now editor explode :)
Isn't a nice way to find another place to apply this optimization? 😄
I will fix later today.

[gziolo]

Added 6eb6826 which fixes the previous issue.

[aduth]

Less included refactoring is better if we want it merged sooner than later.

[gziolo]

@aduth, it's ready for another review.

[catehstn]

Moving this to 4.6, no rush to get it in today.

[gziolo]

Tested the latest changes with the following scenario:

1. Open an existing post (can be the demo).
2. Select all blocks (cmd+a twice) and copy them using cmd+c.
3. Paste them in the new empty post with cmd+v.
4. Select all blocks again and cut them using cmd+x.
5. Paste them in the new empty post with cmd+v.

[aduth]

https://github.com/WordPress/gutenberg/pull/11899/files#diff-32607347f8126e6534ebc7ebaec4853d

😄

[gziolo]

Yes, I've been seeing it every now and then for a few weeks. It's very annoying behavior of npm. Probably some subtle bug recently introduced :(

[jsnajdr]

@gziolo I know I'm very late to the party -- I found this PR when reviewing the @wordpress/* package upgrade in Calypso and wondering why the data package got a 4.0 to 4.1 bump -- but:

If an action needs to know the latest state, that's a very common use case for Redux thunks:

const startSale = () => ( dispatch, getState ) => {
  const stockNumber = getStockNumber( getState() );
  const discountPercent = stockNumber > 50 ? 10 : 20;
  return dispatch( { type: START_SALE, discountPercent } );
};

Then, using the startSale action creator and dispatching the result always uses the latest value in real time.

Just FYI about what the classic vanilla Redux solution is. I don't know if it would apply to the problem that inspired this PR, I just hope it would.

[gziolo]

@jsnajdr - what you shared is another way of tackling that and would work for the example we have in the docs. The implementation we shipped was based on a particular use case which helped us to prevent re-render of all blocks when using arrow key presses. See the lines below:
https://github.com/WordPress/gutenberg/pull/11851/files#diff-8b707a252d7b6da3af477595c2ce11dcL687

To apply your proposal to onShiftSelection action, it seems like we would have to update two actions to make it work. Anyways, it might be useful to support both approaches. If you think it would be beneficial to inject state into actions feel free to open PR with a proposal 👍

[youknowriad]

Actually, we already have a way to do that in actions with controls and generators.

[jsnajdr]

Actually, we already have a way to do that in actions with controls and generators.

How would I do it? Create a synchronous control that selects from the registry? Something like this:

controls = {
  GET_STOCK_NUMBER() {
    return select( 'my/store' ).getStockNumber();
  }
};

function* startSale() {
  const stockNumber = yield { type: GET_STOCK_NUMBER };
  return { type: START_SALE, discountPercent };
}

The code I wrote might be completely misguided, I haven't used a control in practice yet 🙂

[youknowriad]

I was thinking of a generic "select" control. Something like that is already used here

https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/entities.js#L83

[aduth]

I'm excited to start playing with this PR to improve performance of our components.

Follow-up task with some preliminary investigation: #12890

[aduth]

I think we lost this useful information in the transition toward docgen-generated documentation, cc @nosolosw

[oandregal]

how so? I see the comment in the readme taken from the JSDoc block.

[aduth]

🤔 I... swear I'd seen this missing from the current documentation. But it's there, you're quite right. Sorry for falsely raising the alarms!

[oandregal]

No need to be sorry, I'm happily porting anything that's missing :) We ought to have great docs!