Skip to content
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

Automatically use a subregistry when using the block editor provider #14678

Merged
merged 3 commits into from
Mar 29, 2019

Conversation

youknowriad
Copy link
Contributor

This PR automatically creates a subRegistry for each block editor provider making it more like "local state" and making the component more reusable.

We opt-out from this behavior using useSubRegistry={ false } in the edit-post module in order to allow access to the core/block-editor store from the global registry in the editor page. (BC)

Testing instructions

  • Check that you can still call selectors wp.data.select( 'core/block-editor' ).getBlocks()
  • Check that the playground work as expected.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested but it all looks good. I"m a little bit behind all the latest refactorings in this area though :)

}

useEffect( () => {
const newRegistry = createRegistry( {}, registry );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand what's happening here:

  • so it creates a new registry and points the passed registry as a parent
  • overrides core/block-editor
  • does some magic with middlewares

}

useEffect( () => {
const newRegistry = createRegistry( {}, registry );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work as well?

Suggested change
const newRegistry = createRegistry( {}, registry );
const newRegistry = createRegistry( { 'core/block-editor': storeConfig }, registry );

This is what I read from:
https://github.com/WordPress/gutenberg/blob/master/packages/data/src/registry.js#L181

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, previously this was not possible because the controls plugins was not built-in but we can do this now. Good suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess we still can't do that yet because we need the returned "store" object to apply middlewares.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the returned "store" object to apply middlewares.

For now, anyways. @nerrad has been working to eliminate this (e.g. #14594).

(No action implied to be needed here)

@gziolo gziolo added [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement. labels Mar 28, 2019
@aduth
Copy link
Member

aduth commented Mar 28, 2019

I think it should be perfectly fine to put useState usage just before useEffect.

This all is pretty confusing to me 😅 Part of that is being new. Wonder if we should be looking to bring in eslint-plugin-react-hooks to make our lives easier.

}

useEffect( () => {
const newRegistry = createRegistry( {}, registry );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the returned "store" object to apply middlewares.

For now, anyways. @nerrad has been working to eliminate this (e.g. #14594).

(No action implied to be needed here)

@aduth
Copy link
Member

aduth commented Mar 29, 2019

A thought occurs to me: Would we want to do something similar for EditorProvider ? Thinking for how separately I've proposed to use it for reusable blocks . If so, is there a concern about all these nested registries? Or would it scale reasonably well?

@youknowriad
Copy link
Contributor Author

@aduth I created this issue for the react hooks eslint config #14676

@youknowriad
Copy link
Contributor Author

Would we want to do something similar for EditorProvider ? Thinking for how separately I've proposed to use it for reusable blocks . If so, is there a concern about all these nested registries? Or would it scale reasonably well?

I think we should yes, about the scaling, it's a good question that I can't answer :). I'd say that the overhead of a new registry is pretty small based on how it works but I have no proof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants