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

Format library: Allow explicit registration when using npm packages #11611

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 8, 2018

Description

At the moment we register all core format types right away when @wordpress/format-library is executed as discussed here #10209 (comment):

@gziolo: 'm wondering if we should register core formats right away instead. In effect, don't offer this method at all.

@aduth: 💯 registerCoreBlocks is a necessary hack, and certainly shouldn't serve as an ideal precedent.

Related code:
https://github.com/WordPress/gutenberg/blob/master/packages/format-library/src/index.js#L18-L25

This is not ideal when consuming npm package. This PR was opened to discuss alternative approaches. The idea is to offer more granular integration points for the usage outside of WordPress.

This PR explores the following items:

  • getCoreFormatTypes returns an array with all core format types in case someone would want to filter out some of the formats. The shape it returns would have to be defined, I opted for the simplicity of refactoring :)
  • registerCoreFormatTypes method registers all core formats as it happens today.
  • in WordPress registerCoreFormatTypes is executed right after wp-format-library is enqueued which basically mirrors what happens today.

Whatever we decide here, should be also applied to @wordpress/block-library for consistency.

@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Nov 8, 2018
@gziolo gziolo self-assigned this Nov 8, 2018
'wp-format-library',
'wp.formatLibrary.registerCoreFormatTypes();',
'after'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we make sure someone use the edit-post module (via npm) get the formats by default?

Can we remove the wp_enqueue_script (and style) in favor of an explicit dependency towards wp-format-library in wp-edit-post?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it mean, we would call registerCoreFormatTypes in edit-post instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, It's not perfect because people that want to unregister these styles would need edit-post as a dependency. I'd love other opinions.

@ellatrix
Copy link
Member

ellatrix commented Nov 8, 2018

Thanks for taking care of this.

link,
strikethrough,
].map(
( { name, ...settings } ) => [ name, settings ]
Copy link
Member

Choose a reason for hiding this comment

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

Why did we land on an array return value here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The shape it returns would have to be defined, I opted for the simplicity of refactoring :)

We can make it { name, settings } or reduce everything to { [ name ]: settings, ... }. I'm fine with all of them.

import {
registerFormatType,
} from '@wordpress/rich-text';
export function getCoreFormatTypes() {
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it mean we are good with the proposal?

@aduth aduth mentioned this pull request Nov 9, 2018
4 tasks
@aduth
Copy link
Member

aduth commented Nov 12, 2018

#7718 is relevant to consider here, as it includes a new "core" format registered immediately in the @wordpress/annotations package.

I'm on the fence with these functions for registering as it's unclear whether we should consider the idea of registration in tandem with activation, vs. the mere availability of a thing. In practice, we don't make the distinction, which is maybe why we've needed to consider making it an explicit step to trigger the registration.

Could we have more context to this:

This is not ideal when consuming npm package.

?

@gziolo
Copy link
Member Author

gziolo commented Nov 12, 2018

This is not ideal when consuming npm package.

I can see the following issue. You can have more than one version of the same package installed from npm, so the issue boils down to using pure functions rather than the global registry to avoid potential conflicts. We had such reports in the past with Calypso loading 2 different versions of @wordpress/data at the same time. More future related question is what if you would want to have 2 instances of the editor with a different set of controls? How can you pass some context during registration of those format controls?

I'm on the fence with these functions for registering as it's unclear whether we should consider the idea of registration in tandem with activation, vs. the mere availability of a thing. In practice, we don't make the distinction, which is maybe why we've needed to consider making it an explicit step to trigger the registration.

For WordPress plugins, it's convenient to register them right away as site owners can opt-in for their usage. In WordPress core we need to provide some way to make it customizable. The same applies to npm where I would expect even more control because you assume it's for very advanced usage.

@ellatrix ellatrix added [Package] Format library /packages/format-library and removed [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Jan 29, 2019
@gziolo
Copy link
Member Author

gziolo commented Feb 4, 2019

I don't think we reached an agreement on this one. I'm closing it as won't fix. I don't see this as a big issue. We can revisit later as soon as we figure out how to deal with server-side registration for blocks.

@gziolo gziolo closed this Feb 4, 2019
@gziolo gziolo deleted the try/core-format-types branch February 4, 2019 13:30
@gziolo gziolo added the [Status] Not Implemented Issue/PR we will (likely) not implement. label Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Format library /packages/format-library [Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants