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.

Sign up for GitHub

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

Expand on the deprecations documentation #27286

Merged
merged 5 commits into from
Nov 27, 2020
Merged

Expand on the deprecations documentation #27286

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de6635f
Expand on the deprecations documentation
Nov 26, 2020
60ca041
Fix some minor typos and wording
aaronrobertshaw Nov 26, 2020
6b7d115
Changes from feedback
Nov 26, 2020
38ef2b4
Update description of deprecations flow
aaronrobertshaw Nov 27, 2020
8857d6e
Improve clarity of isEligible description
aaronrobertshaw Nov 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion docs/designers-developers/developers/block-api/block-deprecation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,18 @@ When updating static blocks markup and attributes, block authors need to conside

A block can have several deprecated versions. A deprecation will be tried if a parsed block appears to be invalid, or if there is a deprecation defined for which its `isEligible` property function returns true.

Deprecations do not operate as a chain of updates in the way other software data updates, like database migrations, do. At first glance, it is easy to think that each deprecation is going to make the required changes to the data and then hand this new form of the block onto the next deprecation to make its changes. What happens is that each deprecation is passed the original saved content, and if its `save` method produces a valid block it is used to parse the block attributes. If it has a `migrate` method it will also be run, and the data is then passed back to the current block `save` function to create the new valid version of the block.
Copy link
Member

Choose a reason for hiding this comment

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

to the current save function to create the valid version of the block

It's very important to make it more clear that migrate takes attributes parsed from the content of saved block, processes them and runs the original (non-deprecated) save func 😃 I found it confusing myself when helping @ntsekouras to improve the logic of deprecation quite recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reworded this slightly to try and make it clearer, and also re-ordered things slightly so similar concepts were covered in the same spot


It is also important to note that if a deprecation's `save` method does not produce a valid block then it is skipped, including its `migrate` method, even if `isEligible` returns true. This means that if you have several deprecations for a block and want to perform a new migration, like moving content to InnerBlocks, you may need to include the `migrate` method in multiple deprecations for it to be applied to all previous versions of the block.
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the previous paragraph are really nice additions.

The way migrate works is unfortunate, as central aspects of a block deprecation (attributes and save) don't need to be changed once written, which I think leads to this misunderstanding about migrate, but good job on documenting 👍.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's confusing but it makes sense, because it uses attributes and save as they existed when the block was saved. migrate transforms parsed attributes to the shape that the most recent save can consume.

Copy link
Member

Choose a reason for hiding this comment

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

The explanation for isEligible might be not 100% correct. This function checks whether the deprecation should be exercised in the first place. In other words, if this function returns false the deprecation gets ignored. @mcsf, is it correct?

There is also another mention about isEligible earlier in the document so it might need some consolidation.

Copy link
Contributor

Choose a reason for hiding this comment

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

my understanding is that the deprecations are run if the current save produces an invalid block, regardless of the isEligible return ... but if the current save is valid then isEligible is called to see if the deprecation should run anyway:

If I understand the getMigratedBlock function correctly, the block.isValid property is only set to true once a deprecation is successfully applied. So it is not, if the "current save" produces an invalid block but rather if after the deprecations processed so far still leave the block in an invalid state.

Your point that isEligible is not executed in every circumstance, is correct. To me the isEligible provides the means to migrate an otherwise valid block.


For ease of management, it is recommended that you version the deprecations by saving each deprecation into a separate file with the name of the block version it applies to. Then import each of these objects into the `deprecated` property array mentioned below. It is also recommended to keep [fixtures](https://github.com/WordPress/gutenberg/tree/master/packages/e2e-tests/fixtures/blocks) which contain the different versions of the block content to allow you to easily test that new deprecations and migrations are working across all previous versions of the block.
Copy link
Contributor

@talldan talldan Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A brief example might be good, as I think it's quite hard to visualize what a 'block version' is. Some might misinterpret this as an explicit version number in the deprecation object, but I think what you're getting at is better semantics using variable names.

I guess this is the only bit of the PR that might need more discussion, as it's not current practice.

I'd be interested in other thoughts. Something I considered once before was to use comments like:

// Initial version of block
// ...
// 1. Deprecation after captions added.

I like the idea of numbering matching the fixtures. Almost makes me think the fixture html file should live near the deprecation file in a subfolder. An advantage of linking them together in some way is that the tests could then be generic deprecation tests that even work on a third party block that's defined the same way.

Could also be nice to link to the readme in the fixtures folder, as they're quite difficult to understand for a newcomer.

Copy link
Contributor Author

@glendaviesnz glendaviesnz Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

thanks @talldan, that is a good idea to expand this with an example ... we went with this approach in the gallery refactor - https://github.com/WordPress/gutenberg/pull/25940/files#diff-04bda7ff65177c115240f624f7b131cb24350a290b83d9f16f0462bc8a111132R4 - what do you think about this as a suggested way of handling it? We copied this from the way some of the jetpack blocks handle deprecations, eg. https://github.com/Automattic/jetpack/tree/master/extensions/blocks/opentable/deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to have some way to indicate which deprecation came first. For core blocks I think I've noticed usually the earliest deprecation is last in the array, but as you say, that's not a clear convention for contributors.

I'm not overly opinionated on the method used to improve this. I anticipate some might not like the idea of splitting the deprecations into multiple files, but I personally am ok with that. Having said that, the same system could easily be used within one file. e.g.:

const v1 = { // ... };
const v2 = { // ... };

const deprecated = [ v1, v2 ];
export default deprecated;

Worth pinging others on this like @gziolo who's done loads of good work on improving the block structure and bringing consistency across blocks.

Copy link
Member

Choose a reason for hiding this comment

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

First of all, I'm not aware of any recommendation about using a separate file for each deprecation. I never saw it materialized in the codebase as well. Can you expand on that part?

I'm not overly opinionated on the method used to improve this.

If it you think it makes sense like in the linked PR for Gallery block then it's fine. In other places it might create some challenges how to reuse attributes or migration code. Just saying that it have pros and cons. Sometimes hardcoded data would be better but developers hate that 🤷‍♂️

The order of deprecations is important. New deprecation should be always added as a first item in the array (prepended). In effect, the block editor tries all versions of the block in the historical order, from the recent one to the oldest. In the example shared, it should be:

const v1 = { // ... };
const v2 = { // ... };

const deprecated = [ v2, v1 ];
export default deprecated;

It's also how HTML fixtures are named. Version 1 is always the oldest deprecation.

Almost makes me think the fixture html file should live near the deprecation file in a subfolder. An advantage of linking them together in some way is that the tests could then be generic deprecation tests that even work on a third party block that's defined the same way.

There are fixtures for deprecations and current implementation. Actually both could live in the fixtures folder of the corresponding block. I like this idea. I'm not sure how much work it would be but it's something worth exploring. ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this feedback I think that keeping in a single file, but assigning each to a const may be an easier step than putting each in its own file, so have reworded and provided and example for this ... and have also refactored the gallery PR to use a single file instead of multiple.


Deprecations are defined on a block type as its `deprecated` property, an array of deprecation objects where each object takes the form:

- `attributes` (Object): The [attributes definition](/docs/designers-developers/developers/block-api/block-attributes.md) of the deprecated form of the block.
- `supports` (Object): The [supports definition](/docs/designers-developers/developers/block-api/block-registration.md) of the deprecated form of the block.
- `save` (Function): The [save implementation](/docs/designers-developers/developers/block-api/block-edit-save.md) of the deprecated form of the block.
- `migrate` (Function, Optional): A function which, given the old attributes and inner blocks is expected to return either the new attributes or a tuple array of `[ attributes, innerBlocks ]` compatible with the block.
- `migrate` (Function, Optional): A function which, given the old attributes and inner blocks is expected to return either the new attributes or a tuple array of `[ attributes, innerBlocks ]` compatible with the block. As mentioned above, a deprecation's `migrate` will not be run if its `save` function does not return a valid block so you will need to make sure your migrations are available in all the deprecations where they are relevant.
Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is very clear here and in alignment with my previous comments 👍

- `isEligible` (Function, Optional): A function which, given the attributes and inner blocks of the parsed block, returns true if the deprecation can handle the block migration even if the block is valid. This function is not called when the block is invalid. This is particularly useful in cases where a block is technically valid even once deprecated, and requires updates to its attributes or inner blocks.

It's important to note that `attributes`, `supports`, and `save` are not automatically inherited from the current version, since they can impact parsing and serialization of a block, so they must be defined on the deprecated object in order to be processed during a migration.
Expand Down