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

Try a generic block editor module #13088

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@youknowriad
Copy link
Contributor

youknowriad commented Dec 24, 2018

With Phase2, we'd have to load the block editor in several contexts without a dependency to a post object, this PR experiments splitting the current editor module into two module "block-editor" and "editor":

  • The "block-editor" module allows to build a generic block editor component.

<BlockEditor value={ blocks } onChange={ onChange } settings={ settings } />

  • The "editor" module uses this generic component and implements a "post" dependent editor module which handles: saving, dirtiness, editing post properties...

Some decisions/notes about this PR

  • The editor store is splitted into two stores: The block-editor stores holds all blocks's related state (blocks, order, selection, undo/redo...)
  • The post editor store holds all what's related to the post and the save workflow
  • On each change happening the onChange callback is called which ensure we keep track of the blocks in the editor store as well (but without any granularity). An alternative here would be to just call edit( { content: serialize( blocks ) } ) instead but I didn't want to serialize on each change.
  • All block-editor actions and selectors have a "proxy" action/selector in the editor module for backward compatibility.
  • I the moment, I didn't move the "block-editor" related components (think BlockList, BlockListBlock, BlockInserter....). All of these should target the core/block-editor store directly (instead of the proxies) and should live in the block-editor module but I thought I leave this for a separate PR to try to limit the size of the current PR.

Todo

  • Fix the reusable blocks
  • Fix the post meta attributes (I'm thinking of using the editor settings for that)
  • Fix the reusable blocks in the inserter (I'm thinking of using the editor settings here as well)
  • Fix all the tests
  • Check the performance implications. I'm expecting a small loss in performance, but I'm hopeful that we can keep it controlled.

Follow-up tasks (issues to create)

  • Polish the custom sources proposal (used for meta)
  • Avoid using the proxy selectors/actions in the block related components (use core/block-editor instead of core/editor)
  • Move the Block Editor components to the Block Editor Module and leave in the Post Editor only the components specific to the "Post" Editor .
  • Implement the reusable blocks as an embeddable editor (ideally also, remove the "temporary id" state from the reusable blocks and save them only when we hit save the first time)

Overall, I'm feeling optimistic that we can achieve this. I was thinking about how we can split this PR further and land parts of it before, but it seems very hard to do in multiple steps.

@youknowriad youknowriad requested review from WordPress/gutenberg-core , aduth and noisysocks Dec 24, 2018

Show resolved Hide resolved lib/client-assets.php Outdated
value={ blocks }
onChange={ updateEditorBlocks }
settings={ settings }
>

This comment has been minimized.

@youknowriad

youknowriad Dec 24, 2018

Contributor

Here's how you'd use a BlockEditorProvider component, inside it you'd probably have to use a BlockInserter, BlockList and BlockInspector components to build your UI.

@youknowriad youknowriad self-assigned this Dec 24, 2018

@youknowriad youknowriad added this to the Future: Phase 2 milestone Dec 24, 2018

@youknowriad youknowriad force-pushed the try/reusable-block-editor-module branch from 99f2ed0 to 95e31b8 Dec 24, 2018

@youknowriad youknowriad force-pushed the try/reusable-block-editor-module branch from 95e31b8 to a34bda1 Dec 25, 2018

onMetaChange: props.onMetaChange,
editorSettings: {
...props.settings,
__experimentalMetaSource: {

This comment has been minimized.

@youknowriad

youknowriad Dec 25, 2018

Contributor

This is a first iteration of what could be a generic way to declare custom attribute sources. I didn't want to push the proposal a lot. I think we should just land the minimal version here and work on it (polish it) as a separate PR.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

I sent some time testing this PR, and the results look very promising. I did not found any regression on this PR. I was able to find some bugs, but when I tested in master, they were also there.
Congratulations this is an excellent Job 👍
I will post my first superficial thoughts on this PR, I still have many things to look at, it would be perfect if we could divide the PR to make reviewing more manageable, but I'm not sure how the division would like.

We probably need to decide if we are comfortable shipping these changes in a WordPress release before phase 2 or if these changes will be behind some feature flag (may be complicated to do and may increase maintainability effort). If shipping in a minor release we probably need to signal that the API exposed here are all experimental (components, stores, etc...) given that they may be subject to change.

Regarding the performance, I noticed that we are executing a DO_NOTHING action on the editor store on each attribute change. I think this is temporary and as soon as we use the right stores, this will stop happening. Plugins may still be triggering the old actions, which makes me question if it would be possible to have an action deprecation approach that does not forces the execution of a non-required action (calling the reducers). I can think of a middleware that discards the original action and calls the new store, but I don't believe middlewares are an option for us. Another option is having a top-level higher order reducer that just ignores the DO_NOTHIG action without calling any other reducer.

Regarding the performance, I noticed that in this PR when typing we may trigger the re-render of a big component containing all the block editor functionality:
dec-28-2018 16-07-53. This may not be problematic as long as none of the children of that main component rerender.

*/
function computeProviderStateFromProps( props ) {
return {
settings: props.settings,

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 28, 2018

Member

On react blog it says:

If you’re using derived state to memoize some computation based only on the current props, you don’t need derived state. See What about memoization? below.

I think in this case we are only using the derived state to make sure the editorSettings we pass references the same object if the props it depends on did not change.
I think we can use memoization and just memoize a getEditorSettings function, similar to the sample available in https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization.

@noisysocks
Copy link
Member

noisysocks left a comment

I'll need some more time to review properly but have a few comments to start 🙂

) )
);

return createEditorElement( children );

This comment has been minimized.

@noisysocks

noisysocks Dec 31, 2018

Member

There's only two providers here, so I think that using plain old JSX would be easier to understand.

return (
	<SlotFillProvider>
		<DropZoneProvider>
			{ children }
		</DropZoneProvider>
	</SlotFillProvider>
);
super( ...arguments );
props.updateEditorSettings( props.settings );
props.initBlocks( props.value );
this.persistedValue = props.select( 'core/block-editor' ).getBlocks();

This comment has been minimized.

@noisysocks

noisysocks Dec 31, 2018

Member

Can we use props.blocks here instead of calling select()? Or is this is a workaround? If so, a comment would be helpful.


if (
this.props.blocks !== prevProps.blocks &&
this.props.blocks !== this.persistedValue

This comment has been minimized.

@noisysocks

noisysocks Dec 31, 2018

Member

I'm having a hard time understanding this. When would the following expression be false?

( this.props.blocks !== prevProps.blocks ) === ( this.props.blocks !== this.persistedValue )

That is, why can't we remove the second condition in the if ( )?

if ( this.props.blocks !== prevProps.blocks ) {
withSelect( ( select ) => {
return {
blocks: select( 'core/block-editor' ).getBlocks(),
select,

This comment has been minimized.

@noisysocks

noisysocks Dec 31, 2018

Member

Can we call the global wp.data.select() function from the component instead of having select passed along like this? select isn't "data"—I'd worry about other developers copying our example here.

};
}

export function startMultiSelect() {

This comment has been minimized.

@noisysocks

noisysocks Dec 31, 2018

Member

Unrelated: Would be good to set up a lint rule that catches missing JSDoc comments on exported functions that begin with a lowercase letter.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 2, 2019

Great work exploring the block editor module 💯

I like that you are trying to offer a new npm package which would contain a version of the editor which could be run standalone ❤️

With all those bits moved to their own module (actions, selectors and also components in the future), what is your vision for assuring backward compatibility?

Edit: I finally found the code which proxies all actions and selectors as described in the PR:

All block-editor actions and selectors have a "proxy" action/selector in the editor module for backward compatibility.

Can you think of any downsides of using this approach? We never exposed in docs the names of actions so I assume it's okey to dispatch DO_NOTHING for all moved actions. However, this might be used by plugins for some reasons - effects maybe? Anyways, what benefits did you identify which make it appealing to move the code to their own module knowing that it has some technical challenges?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 3, 2019

Can you think of any downsides of using this approach? We never exposed in docs the names of actions so I assume it's okey to dispatch DO_NOTHING for all moved actions.

These names can't be accessed unless you do some "magic" like our "applyMiddlewares". I don't consider these names or the shape of the actions as APIs (we never did).

So yes, I consider these proxies as a good upgrade path. Ideally we'd find a way to remove them at some point but this should be handled like any other deprecation (we should first discuss the strategy for core deprecations).

The fact that all the tests are passing while I didn't rewrite any of our component is pretty telling that these proxies work well to ensure backward compatibility.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 3, 2019

Move the Block Editor components to the Block Editor Module and leave in the Post Editor only the components specific to the "Post" Editor.

This makes me think whether technically we shouldn't do the opposite and move out all pieces that are specific to the post editor. If you take a look at the list of selectors and actions they all are a post related (plus reusable blocks) after all the changes applied: https://github.com/WordPress/gutenberg/blob/f19c29377545dfdf9e1d35a279fc75a7f0911bcf/docs/designers-developers/developers/data/data-core-editor.md

To make it clear. This would be probably harder to make it backward compatible so it's more on the conceptual level than something I think we should try. It seems like the proposed split is easier in many ways. I just regret we didn't do it earlier as now we will have to live in this new proxy based world :)

@youknowriad youknowriad force-pushed the try/reusable-block-editor-module branch from 38d373d to e8915a1 Jan 18, 2019

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