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

Framework: Support registering and invoking actions in the data module #5137

Merged
merged 8 commits into from Feb 22, 2018

Conversation

5 participants
@youknowriad
Contributor

youknowriad commented Feb 19, 2018

The same way we support registering and calling selectors, this PR adds support for registering and calling actions to the data module.

  • We now have a wp.data.dispatch function taking a key and an action (similar to select)
  • We have a wp.data.registerActions to register the exposed actions.
  • We also have support for mapDispatchToProps in the query HoC.

I implemented inserting a block as an exposed action in this PR.

As a follow-up, we could provide a registerState helper function to register everything at once: reducer, actions and selectors.

Testing instructions

  • Open the console of your browser
  • Type wp.data.dispatch( 'core/editor' ).insertBlocks( wp.blocks.createBlock( 'core/image' ) ); in your browser.
  • A new image block should be created.

@youknowriad youknowriad self-assigned this Feb 19, 2018

@youknowriad youknowriad requested review from mtias, gziolo and aduth Feb 19, 2018

wp.data.registerActions( 'myPlugin', { setTitle: ( newTitle ) => ( { type: 'SET_TITLE', title: newTitle } );
```
#### Example:

This comment has been minimized.

@gziolo

gziolo Feb 19, 2018

Member

It's copied from registerSelectors :)

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 19, 2018

In theorhy it should have a public API like:

wp.data.dispatch( 'core/editor' )(
    wp.data.actions( 'core/editor' ).insertBlocks( wp.blocks.createBlock( 'core/image' )
);

to mirror what Redux does. However I like the shortcut you propose here.

@gziolo

gziolo approved these changes Feb 19, 2018 edited

I'm very happy to see it. We have recently discussed with @Shelob9 in #5012, how important it is to make it possible to dispatch actions exposed by core and other plugins. I think this PR concludes the initial version of data module 👏

We might add some safety checks in the follow-up PR which make sure that code will continue to work even when an action or selector got removed by one of the plugins.

export function dispatch( reducerKey ) {
	return actions[ reducerKey ];
}

This will obviously error when someone calls:

wp.data.dispatch( 'remove-key-reducer' ).myAction();

We need to find a way to leave feedback for developers but at the same time make sure that the editor continues to work.

@gziolo gziolo added this to To do in Extensibility via automation Feb 19, 2018

@gziolo gziolo moved this from To do to In progress in Extensibility Feb 19, 2018

@Shelob9

This comment has been minimized.

Contributor

Shelob9 commented Feb 19, 2018

I think this is great. And agree this resolves #5012

I think that @gziolo point about needing to prevent wp.data.dispatch( 'remove-key-reducer' ).myAction(); from causing an error is super important. That protects against a situation where plugin A registers actions, plugin B extends plugin A by using it's actions and then for whatever reason plugin A, but not plugin B is disabled. I've supported 2 different large plugins with add-ons, this is real.

I'd love if a test could be added to ensure dispatching a non-existant action is dispatched doesn't make an error to cover that case.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 19, 2018

wp.data.dispatch( 'remove-key-reducer' ).myAction(); How would you intercept this call to warn about it? myAction could be anything here?

@aduth

This comment has been minimized.

Member

aduth commented Feb 19, 2018

This has been on my mind as well. One thing I was wondering is whether it could make sense to have two separate higher-order components for selecting and dispatching. We could mirror React-Redux connect here, but in my experience I've often used one or the other of the two main arguments and not necessarily both.

Thoughts?

@gziolo gziolo requested a review from mcsf Feb 19, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 19, 2018

wp.data.dispatch( 'remove-key-reducer' ).myAction(); How would you intercept this call to warn about it? myAction could be anything here?

This one is tricky, but for query we can wrap mapSelectorsToProps and mapDispatchToProps with try & catch. We could experiment with Proxies as @mcsf suggested the other day (if we support IE, we would need to use polyfill: https://github.com/GoogleChrome/proxy-polyfill).

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 19, 2018

This has been on my mind as well. One thing I was wondering is whether it could make sense to have two separate higher-order components for selecting and dispatching. We could mirror React-Redux connect here, but in my experience I've often used one or the other of the two main arguments and not necessarily both.

We can also expose the following:

export const actions = mapDispatchToProps => query( undefined, mapDispatchToProps ); 

which would cover all use cases. I'm fine with all possibilities discussed 👍

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 19, 2018

export const actions = mapDispatchToProps => query( undefined, mapDispatchToProps );

No strong opinion for me on whether to expose this or not. A smaller preference towards a single API because it's similar to connect and it's not that harmful to pass undefined as a first param.

@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

The other small worries I have are:

  • The word "query" implies to me no mutation will occur
  • Naming conventions for higher-order components. Do we care to introduce some standards around these, like discussed in implementation notes of #4815 (comment) ?
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 20, 2018

@aduth I'd be open to a name change, what do you propose? something like withData?

As I said, I'm not strongly against having three HigherOrderComponents. Let's discuss in #core-js

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 20, 2018

Yes, all options have pros and cons. Let’s pick one at the core-js chat 👍

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 20, 2018

Per discussions in #core-js slack, I introduced the onSubscribe (name subject to change) Higher Order component as an alternative to query.

  • Only one callback. mapDataToProps
  • Accepts a single argument ownProps
  • We should use wp.data.select and wp.data.dispatch in the callback
  • There's an optimization avoiding to call this callback when props change if the callback doesn't depend on props.
  • There's no optimization support yet for callbacks using props in inline functions inside the callback.
@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

I've been thinking about this since the JavaScript meeting this morning, and I think the main complication comes from the fact that functions created within mapDispatchToProps are often created with new references and incur undesirable re-renders. React-Redux includes some internal optimizations to check for props access to try to avoid running mapDispatchToProps when possible, as we've mimicked here, but it's not perfect.

We also talked about how consolidating to one single-argument higher-order component could help avoid using mergeProps (the third argument of connect), which suffers the same issues as mapDispatchToProps without the same escape hatches. But in fact, this can also be solved by having separate higher-order components, since props generated by the separated higher-order component equivalent of mapStateToProps would flow into the mapDispatchToProps equivalent.

Taking both these points into consideration, I have in mind a separate higher-order component withDispatch which, unlike connect, doesn't expect a function argument returning a new object of props on every state change, but rather accepts a single shared instance of an object, where keys correspond to prop name, the values of which can either be simple action creators or can opt into receiving props in a thunk-like fashion.

Taking the examples here, this could look like:

export default compose( [
	withData( () => ( {
		title: select( 'myPlugin' ).getTitle(),
	} ),
	withDispatch( {
		updateTitle: dispatch( 'myPlugin' ).setTitle,
	} ),
] )( Component );

And for the complex example raised in today's meeting:

const { insertBlocks, updateBlockAttributes } = dispatch( 'core/editor' );

export default compose( [
	withDispatch( {
		onInsertBlocks: ( blocks, insertIndex ) => ( props ) => {
			const { rootUID, layout } = props;

			if ( layout ) {
				// A block's transform function may return a single
				// transformed block or an array of blocks, so ensure
				// to first coerce to an array before mapping to inject
				// the layout attribute.
				blocks = castArray( blocks ).map( ( block ) => (
					cloneBlock( block, { layout } )
				) );
			}

			return insertBlocks( blocks, insertIndex, rootUID );
		},
		updateBlockAttributes,
	} ),
	// ...
] )( BlockDropZone );

This relies on the fact that mapDispatchToProps really only needs to be "calculated" when it's invoked. There's no need to recreate it on every state change. Rather, we override it once, and when it's called, we determine based on the return value of value function how to handle it:

  • Function? Invoke with props
  • Object? Dispatch

One difference with mapDispatchToProps is that you don't have access to dispatch directly, but I can't think of a scenario where I've really needed this. It feels as though it would be useful in the sorts of cases where thunks might be useful, but we already have patterns in place in lieu of this.

props could include all of the current props of the component, or those passed in via previous higher-order components in the compose flow. I expect the latter would be easiest based on what I have in mind for an implementation.

This means we should be able to rewrite current usage of mergeProps with something more like:

const {
	showInsertionPoint,
	hideInsertionPoint,
	insertBlock,
	replaceBlocks,
} = dispatch( 'core/editor' );

export default compose( [
	withData( {
		insertionPoint: select( 'core/editor' ).getBlockInsertionPoint,
		selectedBlock: select( 'core/editor' ).getSelectedBlock,
	} ),
	withDispatch( {
		showInsertionPoint,
		hideInsertionPoint,
		onInsertBlock: ( item ) => ( props ) => {
			const { insertionPoint, selectedBlock } = props;
			const { index, rootUID, layout } = insertionPoint;
			const { name, initialAttributes } = item;
			const insertedBlock = createBlock( name, { ...initialAttributes, layout } );
			if ( selectedBlock && isUnmodifiedDefaultBlock( selectedBlock ) ) {
				return replaceBlocks( selectedBlock.uid, insertedBlock );
			}
			return insertBlock( insertedBlock, index, rootUID );
		},		
	} ),
] )( Inserter );
  • Above example includes some optional shorthand for withData we could consider when not selectors don't need to use props
  • In this usage, the naming dispatch might not be most appropriate, since the return value would be the action object, not yet dispatched (instead coordinated through withDispatch)
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 20, 2018

@aduth
in this example

withDispatch( {
	showInsertionPoint,
	hideInsertionPoint,
	onInsertBlock: ( item ) => ( props ) => {
		const { insertionPoint, selectedBlock } = props;
		const { index, rootUID, layout } = insertionPoint;
		const { name, initialAttributes } = item;
		const insertedBlock = createBlock( name, { ...initialAttributes, layout } );
		if ( selectedBlock && isUnmodifiedDefaultBlock( selectedBlock ) ) {
			return replaceBlocks( selectedBlock.uid, insertedBlock );
		}
		return insertBlock( insertedBlock, index, rootUID );
	},		
} ),

how do you differentiate the more direct handlers hideInsertionPoint, showInsertionPoint and the indirect one onInsertBlock internally in order to provide the right props for the underlying component?

@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

My initial thinking is that what's passed into the component is a proxying function which calls the original value, and only until it returns an object does it dispatch. Otherwise it recursively calls with props of the component.

Let me see if I can put together some (pseudo-)code.

@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

Might look something like this?

export const withDispatch = ( propsToDispatchers ) => ( WrappedComponent ) => {
	class ComponentWithDispatch extends Component {
		constructor() {
			super( ...arguments );

			this.proxyProps = mapValues( propsToDispatchers, ( dispatcher, propName ) => {
				return this.proxyDispatch.bind( this, propName );
			} );
		}

		proxyDispatch( propName, ...args ) {
			const result = propsToDispatchers[ propName ]( ...args );
			if ( typeof result === 'function' ) {
				result( this.props );
			}
		}

		render() {
			return <WrappedComponent { ...this.props } { ...this.proxyProps } />;
		}
	}

	return ComponentWithDispatch;
};
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 20, 2018

@aduth The problem with this implementation is that if someones is extending its store to use thunk middleware which could be a common use case, it will break because action creators will return functions in this case as well.

Also, I'm concerned about the learning curve of this API. It's not that straightforward to understand, especially if you're not familiar with functional programming aspects (currying), redux-thunk and similar.

Edit: Our action creators are auto-bound so maybe we won't have the problem with redux-thunk but we're assuming that dispatching actions don't return functions which is not guaranteed even if it's not common.

@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

Also, I'm concerned about the learning curve of this API.

Sure, it's easy to paint as complex, but we've been stressing the edge cases in this discussion. Going back to the more-common original simple example, it seems pretty intuitive to me:

export default compose( [
	withData( () => ( {
		title: select( 'myPlugin' ).getTitle(),
	} ),
	withDispatch( {
		updateTitle: dispatch( 'myPlugin' ).setTitle,
	} ),
] )( Component );
@aduth

This comment has been minimized.

Member

aduth commented Feb 20, 2018

@aduth The problem with this implementation is that if someones is extending its store to use thunk middleware which could be a common use case, it will break because action creators will return functions in this case as well.

I'd have to see how it plays out, but if we're assuming that end-developers are working with a prebound dispatch function, the result would be an object (the result of store.dispatch) (or undefined?), but even in case of thunk, not a function:

https://github.com/gaearon/redux-thunk/blob/4f96ec0239453623adde857b7e7ad8c4f2897bf1/src/index.js#L4

I expect the return value of this would be undefined in most cases.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 20, 2018

Agreed this is intruitive but in comparison with this:

withSubscribe( () => ( {
	title: select( 'myPlugin' ).getTitle(),
	updateTitle: dispatch( 'myPlugin' ).setTitle,
} ) ),

It seems this one is a bit more simple.
And this is fine enough don't you think?

withSubscribe( ( props ) => ( {
	title: select( 'myPlugin' ).getTitle(),
	updateTitle: memoize( [ props.dep ], ( dep ) => () => dispatch( 'myPlugin' ).setTitle( dep ) ),
} ) ),
@aduth

This comment has been minimized.

Member

aduth commented Feb 22, 2018

Added props change re-selection in 1bcf2e4

@aduth

This comment has been minimized.

Member

aduth commented Feb 22, 2018

Seeing some unexpected re-renders with withDispatch usage. Going to spend a bit more time debugging.

@aduth

This comment has been minimized.

Member

aduth commented Feb 22, 2018

Results of debugging:

  • Inserter rerenders frequently on block changes because we pass the selectedBlock object. While it's not used by the component itself, it's used by withDispatch, and needs to be in its object form. Technically we could call on select directly within withDispatch's mapping function, and this re-begs the question about whether a singular higher-order component would be preferable (since selectedBlock would just be a scoped variable assignment, never passed as a prop), but as has been surfaced in the development of withDispatch, (a) dispatch props don't need to be recreated in response to state changes and (b) it would be difficult to implement the proxying dispatch because in a combined higher-order component we don't know which prop values are dispatching.
    • Conclusion: In the same way the original implementation used the uncommon mergeProps of connect, this is an edge case, where we have a (albeit non-ideal) workaround possible. In most components we'd not be doing this sort of state-dependant dispatch logic, or at least we could hopefully do so without a frequently-changing non-primitive value.
  • There's an "invisible" Inserter which is mounted and unmounted frequently. It's the BlockMobileToolbar component
    • Conclusion: In a subsequent pull request, I want to make this component only render when it is applicable (mobile devices), ideally using the "browser size" Redux state. I could actually imagine moving browser size state out of edit-post and instead registered / managed by a higher-order component isViewportSize (or similar)

@aduth aduth merged commit 9781800 into master Feb 22, 2018

2 checks passed

codecov/project 34.52% (+0.18%) compared to ab55c97
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from In progress to Done Feb 22, 2018

@aduth aduth deleted the add/dispatch-api branch Feb 22, 2018

jorgefilipecosta added a commit that referenced this pull request Feb 23, 2018

Framework: Support registering and invoking actions in the data module (
#5137)

* Framework: Support registering and invoking actions in the data module

* Introduce onSubcribe HoC as an alternative to query

* Store: Register all selectors and action creators

* Data: Separate onSubscribe to withData, withDispatch

* Store: Ensure editor actions / selectors registered early

* Inserter: Convert Inserter to data HOCs

* Data: Update withSelect, withDispatch to invoke callback with select, dispatch

* Data: Re-run selection on props changes
@phpbits

This comment has been minimized.

phpbits commented Feb 28, 2018

@youknowriad Would it be possible to insert rendered blocks? Like:

<!-- wp:paragraph {"align":"right"} -->
<p style="text-align:right">... like this one, which is right aligned.</p>
<!-- /wp:paragraph -->

Thanks!

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 28, 2018

It is possible:

something like

wp.data.dispatch( 'core/editor' ).insertBlocks( wp.blocks.createBlock( 'core/paragraph', { align: 'right', content: [ '... like this one, which is right aligned.' ] } ) );
@phpbits

This comment has been minimized.

phpbits commented Feb 28, 2018

@youknowriad Thanks! So there's a need to convert the blocks first. It's fine though, I just thought I can add a directly html block code. Thanks again!

@phpbits

This comment has been minimized.

phpbits commented Feb 28, 2018

@youknowriad Sorry, another one, How can I use CreateBlock for columns with values. Thanks!

@phpbits

This comment has been minimized.

phpbits commented Feb 28, 2018

@youknowriad nevermind my questions above :) Just figured it out! :)

@phpbits

This comment has been minimized.

phpbits commented Mar 1, 2018

@youknowriad Another question if you don't mind. Just saw clearSelectedBlock but do you have function to get the selected block index? Thanks!

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Mar 1, 2018

@phpbits yes, there is you can find all the available functions here https://github.com/WordPress/gutenberg/blob/master/editor/store/selectors.js

@phpbits

This comment has been minimized.

phpbits commented Mar 1, 2018

@youknowriad Thank you very much!

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