-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Editor: Implement EntityProvider
and use it to refactor custom sources with a backwards compatibility hook for meta sources.
#17153
Editor: Implement EntityProvider
and use it to refactor custom sources with a backwards compatibility hook for meta sources.
#17153
Conversation
packages/block-editor/package.json
Outdated
@@ -28,6 +28,7 @@ | |||
"@wordpress/blocks": "file:../blocks", | |||
"@wordpress/components": "file:../components", | |||
"@wordpress/compose": "file:../compose", | |||
"@wordpress/core-data": "file:../core-data", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not something we should be doing. Adding core-data
as a dependency to the generic BlockEditor module (WP agnostic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably use the BlockListBlock filter to add this in the Editor package instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea.
I paused when doing this initially, but then went with it, because block-library does depend on core-data now, but that is not WP agnostic as block-editor is.
<ReusableBlocksButtons /> | ||
<ConvertToGroupButtons /> | ||
</BlockEditorProvider> | ||
<EntityProvider kind="postType" type={ post.type } id={ post.id }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes me think later once we have the "full site editing" modes, this entity provider wouldn't be added automatically but only when the "Post" block is used (for the other modes, not the regular one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
} | ||
|
||
return [ attributes, setAttributes ]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we somehow bail early if we detect that the blockType doesn't have any "meta" attribute (for performance reasons).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does that:
if ( Object.values( attributeTypes ).some( ( type ) => type.source === 'meta' ) ) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, I wonder if this improves the editor perfs a bit?
Master:
This Branch:
|
Change: Average time to load: 0.50% 🚀 |
…ces with a backwards compatibility hook for meta sources.
8f3ede8
to
c484874
Compare
return acc; | ||
}, {} ), | ||
...kinds.reduce( ( acc, kind ) => { | ||
acc[ kind.name ] = new Proxy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work in IE11 (Proxy and Reflect), any ideas how we can solve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking here, it feels like there's unwanted complexity here (the whole entities object), it feels like what we need is just a global variable with two keys and where we generate a new context each time the value is inexistant (a map of map for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still want it to not work if there is no entity config to avoid unnecessary confusion in digging for errors.
Here: c04c3f8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, let's just simplify the context per kind/entityName creation and ship.
eedc6e9
to
7e1b750
Compare
*/ | ||
import { defaultEntities, kinds } from './entities'; | ||
|
||
const _entities = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid introducing new coding patterns for consistency (we don't really use _something
in our codebase, I assume this is a way to tell it's a private thinig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed.
const getEntity = ( kind, type ) => { | ||
if ( ! _entities[ kind ] ) { | ||
throw new Error( `Missing entity config for kind: ${ kind }.` ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this check, and just create an empty object, the first time a "kind" is requested, we can simplify furthere (remove the _entities
initialization entirely. I'm not certain it adds much value to try to validate that it's an existing kind because we can also just pass a random entity "type" and it will pass the test (we don't validate entity names).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because type
can be dynamic. kind
cannot, you need an entity config for any of the API to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why "kind" can't be dynamic in the future as well tbh. I still think it's . just useless code but I'm fine shipping that way.
Hey @epiqueras This seems to break the editor for me when I add blocks that use post meta as source for one of their attributes since the 6.6.0 update was released. I've created a small plugin to test this and got the same result:
The error comes from the withMetaAttributeSource filter here Please let me know if I'm doing anything wrong or if there is any way to avoid this until next update if it's really a bug. Thanks |
@razwan would you mind creating an issue to track this? |
@youknowriad done. #17767 |
Is the "prop" in |
Merely as an abbreviation.
…On Thu, Dec 5, 2019 at 12:38 PM Andrew Duthie ***@***.***> wrote:
Is the "prop" in useEntityProp intended merely as an abbreviation of
"property" (as in a property of the entity object), or does it have any
association to a React prop?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17153?email_source=notifications&email_token=AESFA2GAGMWM3AJQD2SFHEDQXFRETA5CNFSM4IOZOY62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGCCECY#issuecomment-562307595>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AESFA2E4GLFGUGMTY4T64WLQXFRETANCNFSM4IOZOY6Q>
.
|
Added a dev note to document the fact that the meta keys are not available in the |
Thanks! |
Description
This PR implements the notorious
EntityProvider
and auseEntityProp
hook, and uses them to refactor custom sources, with a backwards compatibility hook for meta sources.It's the first step in getting the features of #16565 and:
EntityHandlers
component and basic template blocks. #17020EntityProvider
, controlledInnerBlocks
and basic template blocks. #17135shipped 🚀
How has this been tested?
All the current test suites that test meta attribute sources were verified to still be passing.
Types of Changes
New Feature: Add a new
EntityProvider
component and auseEntityProp
hook that allows blocks to edit and sync with different entities, removing the need for custom sources in the process.Checklist: