WIP - Allow plugins to extend core embed blocks #14050
Conversation
notnownikki
requested review from
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
jorgefilipecosta,
nerrad,
noisysocks,
ntwb,
Soean,
talldan and
youknowriad
as code owners
gziolo
added
[Status] In Progress
|
In an ironic twist, it's Jetpack that makes the |
|
@notnownikki it may help to squash commits to proposed changes, then the youtube demo enhancement or pull this out to two PRs with the latter based on this branch. What type of feedback are you interested in? |
|
Would it be possible to rebase? Running this locally I'm seeing, when clicking on a paragraph |
notnownikki
force-pushed
the
update/embed-block-extra-options
branch
from
d45385c to
694336a
Compare
notnownikki
force-pushed
the
update/embed-block-extra-options
branch
from
694336a to
34b62fc
Compare
|
@gwwar I rebased this and now there are two commits, one that adds the extra settings, and one that extends the YouTube block. I'm not sure where that paragraph error is coming from, I don't see it locally, and the e2e tests that run in CI don't see it either. It looks like a usage tracking thing unrelated to this PR, is it appearing in master for you too? Anyway, this should be a lot easier to review now :) I'm looking for a code review, and feedback on the approach and how well it will allow Jetpack to extend core embed blocks to let them be backed by shortcodes and implement all the nice options that the shortcodes have that oEmbed doesn't. I also really need help figuring out why enabling Jetpack changes the order scripts are loaded in and prevents the test plugin |
| }; | ||
| const onChangeStart = ( value ) => { | ||
| this.setAttributes( { start: value } ); | ||
| }; |
There was a problem hiding this comment.
Minor note but we are creating new functions on each render call. The value is being passed from the event, so we can pull this out of render. Were you having problems with this?
There was a problem hiding this comment.
Ugh, no real reason other than took the shortest route to making it work so I could get feedback on the approach :) definitely should be moved out
| constructor() { | ||
| super( ...arguments ); | ||
| const { extraOptions = {} } = this.props.attributes; | ||
| this.setAttributes = this.setAttributes.bind( this ); |
There was a problem hiding this comment.
Maybe name this something else, it's a bit confusing to read vs props.setAttributes
| // because we don't want videos autoplaying in the editor. | ||
| const { start, relatedOnlyFromChannel } = attributes.extraOptions; | ||
| if ( undefined !== start && parseInt( start ) > 0 ) { | ||
| extraQueryParams = extraQueryParams + '&start=' + parseInt( start ); |
There was a problem hiding this comment.
I'm not sure if we have a polyfill for this, but if so, https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams would be useful to build a query string
There was a problem hiding this comment.
| /** | ||
| * External dependencies | ||
| */ | ||
| import classnames from 'classnames/dedupe'; |
There was a problem hiding this comment.
Interesting, not a typo :D https://github.com/JedWatson/classnames#alternate-dedupe-version
"This version is slower (about 5x) so it is offered as an opt-in." Both appear to be in use in Gutenberg.
| } else { | ||
| fetching = isRequestingEmbedPreview( url ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Another minor note here, but may be worth pulling out to helper functions, so it's easier to reason about the control flow:
getPreview = ( ownProps ) => {
const { url } = ownProps;
if ( ! url ) {
return false;
}
if ( options.preview ) {
options.preview( getEmbedPreview( url ), ownProps.attributes );
}
return getEmbedPreview( url );
}
| componentDidUpdate( prevProps ) { | ||
| if ( prevProps.html !== this.props.html ) { | ||
| // Allows the new html to go into the sandbox. | ||
| this.iframe.current.contentDocument.body.removeAttribute( 'data-resizable-iframe-connected' ); |
There was a problem hiding this comment.
For folks reading there's an early exit in sandbox otherwise
|
Thanks @notnownikki, I think this is a pretty interesting and valid use case. I left a few minor notes/thoughts, but overall I'll defer to @WordPress/gutenberg-core for technical guidance here. Some bigger questions:
I suppose if we wanted to add a shortcode fallback in html content, we'd update serialized output. We'd need to make sure that core html attribute parsing is a bit more forgiving, as we'll look for an exact match. Maybe adding a function to the For example: <RawHTML>{ `[myembed id="${ id }"]` }</RawHTML>cc @mmtr in case you're interested in taking a look as well. |
"It depends" - every engineer ever ;) If the YouTube extension was removed, the blocks would continue to work, because we're modifying the output of oEmbed with a server side dynamic block and not altering what gets saved. So editing those would load as expected, because oEmbed would still run, but the extended behaviour wouldn't render any more so you'd just get the normal version. If a plugin was removed that had replaced |
As I understand its history, this is the crux of why extending blocks hadn't previously been implemented, since the story here is not ideal; neither for the user whose blocks are now suddenly invalid, nor for the plugin author who probably won't consider the reality that their plugin may not always be activated. Some options to consider, then:
|
|
@aduth thank you! I'll take another look at how we could do this without extending blocks while still allowing the shortcode-backed block to pick up URL pastes at a higher priority than core blocks that handle the same URLs, as that seems to require the fewest changes and be in line with other efforts. |
|
This shouldn't be too difficult to change round so that plugins can register embed blocks and have them used in the URL paste and embed block resolution. I still need help with figuring out why Jetpack changes the script load order though, it would mean that when Jetpack is active, other plugins would not be able to register their embed blocks because of the reasons in #14050 (comment) |
|
Ok, this has been changed around so that plugins can supply a full set of embed block settings into the common or other embed blocks, using a filter. This lets the plugin provide a fully featured embed block without changing the existing one, and the plugin can decide whether the block takes priority over the existing block when it comes to URL pasting or not, by either putting it at the top or bottom of the list of block settings. In the example e2e plugin, there's an enhanced Reddit block, and both this block and the existing core block are available to the user. The plugin's block takes priority when resolving a URL to an embed block. |
|
This is really cool, @notnownikki! I just wanted to share what the extra settings look like.
I wonder if the number field might look better as a shorter input.
|
|
Thanks 😁 this has had very little visual polish, and I've held off on documentation until we've got the settings approach nailed down and the jetpack conflict sorted, just so you know 🙂 |
|
Some updates here. It seems like #9757 has been open for a while and is a symptom of much the same problem - the registration code runs inline and if something changes round the load order of scripts, then plugins can miss filters and there's nothing they can do about it. I'm looking at other implementations to allow the enhanced blocks to hook into the Pasted URL -> Embed Block resolution code, so we can work around that issue. |
|
I've opened a new issue at #14578 to get more feedback on the underlying problem. |
|
It's working with Jetpack! @aduth your eyes would be very much appreciated on this again :) Things I know are not there yet:
It's been changed so that the filter on the list of blocks used to resolve a URL to an embed block happens during the resolution process, because we can't guarantee that a plugin's code will load before |
notnownikki
added
the
Needs Technical Feedback
|
There's a lot to take in here, so forgive me if points of my comment have already been addressed or considered. This pull request seems to add quite a few things around providing a packaged option for extending blocks behavior, but it also seems relatively constrained in what's actually possible (extending only embeds block). The included example of extending the YouTube block appears to include some specific server behaviors which either a plugin couldn't imitate, or they could only imitate by completely replacing the default server render behavior of the block (i.e. not interoperable with other plugins or core behavior which seeks to do the same). I'm a bit wary also of:
Considering the original goal, it seems there's a lot which already exists to be able to extend the default behavior. Complemented with parts introduced here (like One thing which hadn't occurred to me previously, but which Considering the given YouTube options example, I might wonder then if it's a matter of:
wp.hooks.addFilter( 'blocks.registerBlockType', 'jetpack/youtube-autoplay', ( settings, name ) => {
if ( name !== 'core-embed/youtube' ) {
return settings;
}
return {
...settings,
attributes: {
...settings.attributes,
autoplay: {
type: 'boolean',
default: false,
},
},
};
} );
wp.hooks.addFilter(
'editor.BlockEdit',
'jetpack/autoplay-inspector-controls',
( BlockEdit ) => ( props ) => (
<Fragment>
<BlockEdit { ...props } />
<InspectorControls>
{ /* Autoplay Controls */ }
</InspectorControls>
</Fragment>
)
);
function jetpack_add_youtube_autoplay( $content, $block ) {
if ( 'core-embed/youtube' !== $block['blockName'] ) {
return $content;
}
return sprintf(
'[embed args="%s"]%s[/embed]',
http_build_query( array( 'autoplay' => $block['attributes']['autoplay'] ) ),
$content
);
}
add_filter( 'render_block', 'jetpack_add_youtube_autoplay', 10, 2 );
function jetpack_youtube_block_add_oembed_parameters( $provider, $url, $args ) {
if ( ! empty( $args['args'] ) ) {
wp_parse_str( $args['args'], $extra_args );
$provider = add_query_arg( $extra_args, $provider );
}
return $provider;
}
add_filter( 'oembed_fetch_url', 'jetpack_youtube_block_add_oembed_parameters', 10, 3 );The trickiest part of all of this seems to be managing the oEmbed processing, both as it impacts preview in the editor, and in considering additional arguments for the fetch. The snippet above with the The editor preview is a bit more challenging mostly in how the proxy endpoint currently only supports to receive the URL, and isn't aware of whether that request is coming from the block editor, or the specific context of the block for which it is being requested (including the block's attributes, where
The key with all of this is that with these extensions, nothing about the generated markup of the block changes (not considering the comment attributes, since these are ignored for validation). This is critical to ensure that the content is future-proof against plugin deactivations. |
|
@aduth I agree with a lot of that, but unfortunately it can't be implemented with how the block registration filters get run. There's an old issue #9757 which highlights that the block registration filters sometimes get run too early for plugins to use them. If we look at how assets are loaded when Jetpack is active (issue raised here in Jetpack but closed because Jetpack only reveals the symptom, it's not the cause Automattic/jetpack#11464 ), plugins that rely on those block registration filters to modify settings or inject their own sometimes don't get chance to run, because Jetpack requires The other side of things is that for previewing YouTube, oEmbed does not support the options that this PR enables. Start time, related videos, etc. are not supported by YouTube embed, so we can't pass those options to any of the All of this becomes easy to manage, and a large part of this PR's code disappears, if we can rely on filters running. |
notnownikki
added
the
[Status] Blocked
|
Closing this one out as it would require a large reworking if/when filters are replaced with something else. |
Description
This is Work In Progress, needing feedback from plugin developers and core Gutenberg team members.
Some plugins (e.g. Jetpack) have shortcodes that handle embedding certain content, for example, YouTube. These shortcodes have extra options that aren't available through oEmbed, because the provider's oEmbed API does not support the extra options.
This PR adds new options to the embed block settings to allow plugins to customise core embed blocks and add extra options, modifying the output from oEmbed if needed, or allowing the plugin to override what the embed block saves with a shortcode.
The new options are:
preview: function that returns an enhanced preview. Takes the preview oEmbed generates as an argument.save: function that allows the plugin to override the block's save.fetching: function that returns a boolean to show if thepreviewis being fetched. This allows custom preview endpoints to be implemented.inspector: component to be rendered as part of the block inspector, to provide controls for any new options the plugin provides.This PR also includes enhancements for the YouTube block, allowing us to set relative video options, start time, and autoplay. This might not be suitable for inclusion in core (more suited to a plugin?) but it's here as an example of what's possible with the new options.
To demonstrate how a plugin can provide a custom save and preview, the
packages/e2e-tests/plugins/extend-embeds/plugin shows the Reddit block being backed by the plugin's own preview endpoint.How has this been tested?
There's a new e2e test that has a plugin that demonstrates all the new features are working.
Types of changes
New feature, completely backwards compatible.
Checklist: