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

Editor: Implement EntityProvider and use it to refactor custom sources with a backwards compatibility hook for meta sources. #17153

Merged
merged 6 commits into from
Sep 23, 2019

Conversation

epiqueras
Copy link
Contributor

Description

This PR implements the notorious EntityProvider and a useEntityProp 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:

shipped 🚀

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 a useEntityProp hook that allows blocks to edit and sync with different entities, removing the need for custom sources in the process.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras added [Package] Core data /packages/core-data [Package] Editor /packages/editor [Package] Block editor /packages/block-editor labels Aug 22, 2019
@epiqueras epiqueras added this to the Future milestone Aug 22, 2019
@epiqueras epiqueras self-assigned this Aug 22, 2019
@@ -28,6 +28,7 @@
"@wordpress/blocks": "file:../blocks",
"@wordpress/components": "file:../components",
"@wordpress/compose": "file:../compose",
"@wordpress/core-data": "file:../core-data",
Copy link
Contributor

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).

Copy link
Contributor

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.

Copy link
Contributor Author

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 }>
Copy link
Contributor

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

}

return [ attributes, setAttributes ];
}
Copy link
Contributor

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).

Copy link
Contributor Author

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' ) ) {

Copy link
Contributor

@youknowriad youknowriad left a 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?

@epiqueras
Copy link
Contributor Author

Master:

Average time to load: 4560ms
Average time to DOM content load: 4252ms
Average time to type character: 53.8ms
Slowest time to type character: 129ms
Fastest time to type character: 48ms

This Branch:

Average time to load: 4583ms
Average time to DOM content load: 4297ms
Average time to type character: 47.085ms
Slowest time to type character: 102ms
Fastest time to type character: 41ms

@epiqueras
Copy link
Contributor Author

Change:

Average time to load: 0.50%
Average time to DOM content load: 1.06%
Average time to type character: -12.48%
Slowest time to type character: -20.93%
Fastest time to type character: -14.58%

🚀

@epiqueras epiqueras force-pushed the add/entity-provider-and-refactor-custom-sources branch from 8f3ede8 to c484874 Compare September 16, 2019 18:33
return acc;
}, {} ),
...kinds.reduce( ( acc, kind ) => {
acc[ kind.name ] = new Proxy(
Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Contributor

@youknowriad youknowriad left a 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.

@epiqueras epiqueras force-pushed the add/entity-provider-and-refactor-custom-sources branch from eedc6e9 to 7e1b750 Compare September 20, 2019 15:53
*/
import { defaultEntities, kinds } from './entities';

const _entities = {
Copy link
Contributor

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)

Copy link
Contributor Author

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 }.` );
}
Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@epiqueras epiqueras merged commit 03e034f into master Sep 23, 2019
@youknowriad youknowriad deleted the add/entity-provider-and-refactor-custom-sources branch September 23, 2019 17:13
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.6 Sep 30, 2019
@razwan
Copy link
Contributor

razwan commented Oct 4, 2019

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:

Cannot read property 'my_block_meta' of undefined

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

@youknowriad
Copy link
Contributor

@razwan would you mind creating an issue to track this?

@razwan
Copy link
Contributor

razwan commented Oct 4, 2019

@youknowriad done. #17767

@aduth
Copy link
Member

aduth commented Dec 5, 2019

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?

@epiqueras
Copy link
Contributor Author

epiqueras commented Dec 5, 2019 via email

@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Dec 11, 2019
@youknowriad
Copy link
Contributor

Added a dev note to document the fact that the meta keys are not available in the getBlockAttributes selector anymore. Devs should rely on getEditedPostAttribute instead. See #19067

@epiqueras
Copy link
Contributor Author

Thanks!

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 [Package] Core data /packages/core-data [Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants