Export original MediaPlaceholder component from block-editor package

[desaiuditd]

Description

Currently, we can override the MediaPlaceholder component using the existing JS filter editor.MediaPlaceholder. Below is the example snippet mentioned in the README file of the component:

function replaceMediaPlaceholder() {
    return function() {
      		return wp.element.createElement(
      			  'div',
      			  {},
      			  'The replacement contents or components.'
      		);
	    }
}

wp.hooks.addFilter(
        'editor.MediaPlaceholder',
    	'my-plugin/replace-media-placeholder',
    	replaceMediaPlaceholder
);

Another example is to extend wp.element.Component, create a fresh component from scratch and pass it to addFilter callback. As below:

const { element: { Component } } = wp;

class MyMediaPlaceholder extends Component {
    render() {
        return ( <div>{ 'The replacement contents or components.' }</div> );
    }
}

wp.hooks.addFilter(
	'editor.MediaPlaceholder',
	'my-plugin/replace-media-placeholder',
	() => MyMediaPlaceholder
);

Above two approaches work fine if we want to replace the MediaPlaceholder component with a completely fresh component written from scratch.

But in real world, this would not be ideal. We would want to take the existing MediaPlaceholder component as a base and modify/customize certain behaviour of the component.

In order to achieve this, let's take below approach:

const {
    blockEditor: { MediaPlaceholder },
    element: { Fragment }
} = wp;

class MyMediaPlaceholder extends MediaPlaceholder {
    // Basically, I am disabling the Drag'n'Drop feature of MediaPlaceholder component.
    renderDropZone() {
        return ( <Fragment /> );
    }
}

wp.hooks.addFilter(
    'editor.MediaPlaceholder',
    'my/plugin/replace-media-placeholder',
    () => MyMediaPlaceholder
);

Above approach will not work. It will go into an infinite recursion. Below is the explanation for it.

wp.blockEditor.MediaPlaceholder is not the original vanilla React component. It's an HoC and has gone through the composition of withFilter.

So using that to create a customized component, and passing it again to addFilter will create the infinite recursion.

In order to fix this or make partial customization of original component possible, I'm exporting the original component out of the block-editor package. So that other plugins/consumers can start using it.

I've not changed the existing export of MediaPlaceholder HoC, as it is widely used in the codebase. So I've added a new named export with OriginalMediaPlaceholder.

Happy to listen to other opinions on naming convention or this approach all together.

How has this been tested?

I've verified that the OriginalMediaPlaceholder is available publicly via wp.blockEditor.*. It's not tempering with any of the existing functionality as it's a new export. Unit tests and linting are passing on my local.

Also, Creating a new component by extending OriginalMediaPlaceholder and passing it to the addFilter callback, does not create infinite recursion anymore. Tested this in my local.

Types of changes

New export variable out of block-editor package.

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.

[peterwilsoncc]

Reviewing the components, the wp.blockEditor is a mix of vanilla components and HoCs. A number of items in the namespace will cause infinite loops if they are extended:

• MediaPlaceholder (for the sake of completeness)
• BlockDropZone
• Edit
• BlockListBlock

As/If other components have filters added via the HoC, a similar problem will occur when attempting to extend existing blocks.

While they should be exposed, I'm not convinced exposing the original components within the same namespace as wp.blockEditor.original* is the best approach but I'm also unsure what is. My naive thoughts are either wp.blockEditor.components.* or wp.blockEditorComponents.*.

[desaiuditd]

wp.blockEditor.components.*

I like this approach. Can work towards it. Just need someone to confirm if it is the right way forward.

[talldan]

A few bits of feedback on this:

class MyMediaPlaceholder extends MediaPlaceholder {
    // Basically, I am disabling the Drag'n'Drop feature of MediaPlaceholder component.
    renderDropZone() {
        return ( <Fragment /> );
    }
}

Extending React components this way isn't recommended. A component can be a function or a class, and there's nothing to stop the gutenberg implementation from being refactored from a class to a function. At this point the code would break. Some more details here:
https://reactjs.org/docs/composition-vs-inheritance.html

---

wp.hooks.addFilter(
    'editor.MediaPlaceholder',
    'my/plugin/replace-media-placeholder',
    () => MyMediaPlaceholder
);

The use of the filter here isn't what's intended. This might be the fault of the example in the README (which I'd definitely propose updating.)

The third argument to the filter is the function that's triggered when applyFilter is called. That function is passed the original version of the MediaPlaceholder component for extension. The desired use case is something like:

function enhanceMediaPlaceholder( MediaPlaceholder ) {
    return ( accept, ...props ) => (
        <MediaPlaceholder
            { ...props }
            accept={ [ ...accept, 'image/some-obscure-format' ] }
        />   
    );
}

wp.hooks.addFilter(
    'editor.MediaPlaceholder',
    'my/plugin/enhance-media-placeholder',
    enhanceMediaPlaceholder,
);

Here enhanceMediaPlaceholder is returning a function component that wraps the original MediaPlaceholder and overrides the passed in props to add a new accepted image format (in a real-world scenario there are probably also other parts of the codebase that need some extension for new formats to be accepted.)

Given that the original component is already available through the filter, I don't really see a reason to export the component and I think the PR could be closed. It would be good to know what the original use case is, though. Is it disabling the drop zone as mentioned in the shared snippet?

[peterwilsoncc]

@talldan Thanks for the detailed response.

I'm a colleague of Udit's so can answer your question about the original use case.

Initially, yes, it's to disable to the drop zone per the example in the original message.

We have a custom media frame implementation that has required fields for the user to fill in. The drop zone component allows them to work around the required fields.

Eventually we're hoping to replace or extend the drop zone to open the media library with the uploaded image selected and ready to be inserted once the required fields are filled in. But this is a little way down the track.

Again, thanks for taking the time to respond with such detail.

[talldan]

Ok. That would be tricky to achieve with the current MediaPlaceholder. I think perhaps one way would be to add a disableDropZone prop to the MediaPlaceholder, then either you could use the filter to change existing placeholders or use the component directly with that prop specified if you're implementing a separate interface.

[youknowriad]

I have been wantinng to create a document to explain how do we maintain the Gutenberg APIs, add some guidelines on third-party usage, backward compatibility ... for clarity's sake. Not extanding components is one of the aspect that are important to hightlight I think. I hope I can get to creating this document soon enough. if others can champion this too, that would be lovely.

[desaiuditd]

A component can be a function or a class, and there's nothing to stop the gutenberg implementation from being refactored from a class to a function. At this point the code would break.

Of course. I would only try to extend a component knowing that it's a class after checking the Gutenberg codebase. By doing so, I'm for sure making my code prone to error/broken state. But that's something I'm willing to compromise (with proper functional/unit tests on my end and also observing the Gutenberg development closely and keeping an eye on this component).

That function is passed the original version of the MediaPlaceholder component for extension.

Given that the original component is already available through the filter, I don't really see a reason to export the component

Ah. I completely missed this. Thanks. 😃

I think perhaps one way would be to add a disableDropZone prop to the MediaPlaceholder

Happy to change this PR to reflect this change.

Not extanding components is one of the aspect that are important to hightlight I think.

So we are not supposed to extend Core components ever? Honestly, I've observed time to time that convincing to add any customization/modification to Core WP or relevant project is hard. Because it may not be useful to larger community. And I don't have a problem with that. It completely makes sense to not include anything that is useful for just one small section of community.

But then, just because of that, I would not stop my use case from happening, right? So I would rather extend from what is available, add my changes in my repo and move ahead. All is happy. And that's the philosophy WordPress has followed in Core as well. And that's what I love about it. And that's what makes it successful in my opinion.

[desaiuditd]

I think the PR could be closed.

Can be closed in favour of #17077

[talldan]

So we are not supposed to extend Core components ever?

When mentioning extending components, I think @youknowriad is referring to extending classes, which is generally considered an anti-pattern in the React community for the reasons mentioned, so that's why this is discouraged.

There are lots of other ways to extend functionality in the codebase. I think here the situation is a bit unfortunate because the MediaPlaceholder is one of the larger more complex components—it encapsulates a lot of logic and a lot of user interface. With most other components it's easier to compose them together from smaller parts to build something different. Composition, wrapping, slots and filters are the current main ways to expand and extend. There's also some growing use of React hooks, which I think offers a much simpler way to build functionality.

Honestly, I've observed time to time that convincing to add any customization/modification to Core WP or relevant project is hard.

I totally understand that this is really important, it is a an important ethos of the WordPress project. At the same time there's another perspective. It's easy for the anyone developing core functionality to back themselves into a corner by introducing extensibility in the wrong way. The backwards compatibility of that code has to be managed for the lifespan of WordPress. Doing it in the wrong way can stifle future improvement because the existing implementation has to worked-around when developing anything new. The Gutenberg codebase in particular is young and in rapid flux, so anything added has to be very well thought out.

[desaiuditd]

which is generally considered an anti-pattern in the React community for the reasons mentioned, so that's why this is discouraged.

💯. Noted.

And for the rest of the comment, completely agree. Nothing against the current principles and processes that have been put forward. All good. 👍

[talldan]

Closing in favour of #17077