WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Update docs to reflect new preferred `convert` method on transform objects #15972

Open getdave opened 5 years ago

getdave commented 5 years ago

A new convert method will shortly become the preferred way to define the way in which one (or more blocks) can be transformed into one (or more) other blocks using the Block Transforms mechanic.

Currently developers define a transform method on the transform object definition in which they defined how to transform the block.

transforms: {
    from: [ {
        type: 'block',
        blocks: ['*'],
        transform: (attributes, innerBlocks) => {
            // do the transforming!
        }),
    } ],
},

The issue is that this function is provided with only 2 arguments

  1. Block attributes
  2. Block innerBlocks

However, there are many cases where having access to the entire Block object is required (eg: for example see the Grouping mechanic).

After a lengthy discussion in Slack and having reviewed several options, it was decided to create a new convert method which would accept the full block object.

transforms: {
    from: [ {
        type: 'block',
        blocks: ['*'],
        convert: (blocks) => {
            // do the transforming with all the block data!
        }),
    } ],
},

When the transform is applied this convert method is always run in preference over any transform method. However, if no convert method is defined then the transform method will be attempted. As a result it is fully backwards compatible.

transforms: {
    from: [ {
        type: 'block',
        blocks: ['*'],
        convert: (blocks) => {
            // this will run in preference to the transform method below
        }),
        transform: (attributes, innerBlocks) => {
            // this will NOT be run as convert takes precedence
        }),
    } ],
},

We now need to fully document this change. This involves:

See also https://github.com/WordPress/gutenberg/pull/14908

getdave commented 5 years ago

@youknowriad @aduth Did we decide to stick with this naming?

Either way is this docs task required for the release (which I think is coming up soon right?).

aduth commented 5 years ago

@youknowriad @aduth Did we decide to stick with this naming?

I don't personally take strong issue with it. I know @nerrad had proposed an alternative applyTransform which is clear and respects the act of the application of a transform, but I find to be both verbose and somewhat redundant (with the transforms top-level property, not that the previous transform function wasn't already).

I think I'll be fine to see it go through as-is. We could flag it as "experimental" if we wanted some more time to consider it. But just as well, there is still an option to deprecate before the next WordPress stable release if a viable alternative presents itself.

Either way is this docs task required for the release (which I think is coming up soon right?).

No, I don't think it's required.

aduth commented 5 years ago

After talking a bit more with @nerrad , I think we may want to just mark this as experimental for now. There were a few other approaches we discussed, and in general I don't think we seem all that comfortable to commit to the current API as proposed. Since it's not documented anyways, we can plan to stabilize the API in tandem with introducing the documentation.

A few of the additional considerations:

Personally, I also sought to look up additional options for words, closer to the intention with the "apply" as describe the act of applying a transform. None of which really strike me more than what's already been considered.

I've also considered completely different options for defining the transform, again not striking me as particularly viable or an improvement.

As an immediate action item, I will follow-up with a pull request to mark this function as experimental (Edit: See #16047).

nerrad commented 5 years ago

Thanks for summarizing this @aduth!

getdave commented 5 years ago

@aduth When might be remove __exeperimental? Is there a process for this?

getdave commented 4 years ago

@youknowriad @aduth Sorry to chase, but just wondering if it's ok to make this a first-class citizen now? I noticed this effort to audit experimental APIs so perhaps this will be taken care of without me needing to keep 👁 👁 on it?

aduth commented 4 years ago

@getdave Sorry that I'd missed your October ping, as it happened to fall within a window of my extended leave from the project. Coincidentally, I'd been thinking about this since it had been brought up in Slack as something to stabilize. Much of this prior discussion had slipped my memory, and I don't personally take any issue with stabilizing the function as it exists today.

I can plan to take care of it, unless you had already been thinking about it?

aduth commented 4 years ago

Looking at the documentation:

https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-registration.md#transforms-optional

There are two things which stand out to me:

One idea which I don't know had been considered previously: What if we continue to support transform, and name the new function transformMany? Or some equivalent naming which implies that the intended usage is to convert multiple blocks. In this line of thinking, we don't need to advocate one or the other as being preferred, but each as equally sensible under certain conditions.

There's still a problem in that one form receives the full block objects (transformMany( blocks )) while the other only receives a subset of the block detail (transform( attributes, innerBlocks )). Again, this is where it might have just been preferable to have passed the block object to transform in the first place, but it's not something we can really change at this point in time.

What we could do, though, if we want the behavior to align, is to proceed with the renaming and soft-deprecation of transform, while still keeping the singular vs. many forms as two separate options.

Something like:

The reason I think this matters is that, anecdotally, I suspect that the majority of usage will only care to apply a consistent transformation on singular blocks. The multi-block transforms (like for the Group block) seem to be more of the edge-case, so it doesn't really make sense to me to optimize for this.

aduth commented 4 years ago

Looking again at the implementation, there's already an (undocumented) expectation that multi-block transformations should provide a isMultiBlock: true value as part of the transformation descriptor. Indeed, this is what the group block already provides to ensure that the conversion function receives an array of blocks.

Thus, I don't think we need to have separate functions, we can just name the function convert, and if the transformation also assigns isMultiBlock: true, then it will receive an argument as an array of blocks, otherwise it will receive as a single block.

aduth commented 4 years ago
  • Based on the previously-linked Slack discussion, we might want to consider the impact this has on the shortcode transform API, which is distinct from the block transform process, but adheres more to the API of the transform function (receives attributes as an argument). I suppose it depends whether we'd ever care to support the use-case of converting multiple shortcodes to one or more equivalent blocks (probably not), but in any case, there's now a bit more of a divergence in how the two are used.

Related to this, and probably more pressing for consideration: There are special kinds of transformation "types" which are handled outside the standard block transform API. These also use the transform naming, presumably as an overload of the block transformation transform function. I think for consistency's sake, we'd want to update these as well to prefer the consistent convert naming.

See also:

This late observation and additional maintenance overhead can serve as good justification that this ought to be refactored to a consolidated approach, which might be worth doing as part of this effort.

aduth commented 4 years ago

I managed to work my way through most of a branch to deprecate transform consistently in favor of convert, but encountered another issue in a potential inconsistency it introduces with the transforms isMatch function. The function receives an attributes object as its argument, much like the transform function. Granted, there's already a bit of inconsistency here in that it does not receive innerBlocks as a second argument like the transform function had. We could decide to be content with this disparity. In retrospect, ideally this would receive a block or blocks object(s) just like an ideal transform function would have.

aduth commented 4 years ago

The pull request at #19401 will stabilize this API. The open questions are still pending on how we address some of the inconsistencies noted in my previous comment.

oandregal commented 4 years ago

Cross-commenting for visibility https://github.com/WordPress/gutenberg/pull/19401#issuecomment-618983111