Expand on the deprecations documentation #27286
Conversation
|
Size Change: +71 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
|
||
| 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 recommeded 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. | ||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. ❤️
There was a problem hiding this comment.
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 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. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
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 👍.
There was a problem hiding this comment.
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.
gziolo
added
[Type] Developer Documentation
There was a problem hiding this comment.
Thank you for trying to clarify how deprecation works. It's definitely one of the most complex and therefore confusing aspect of block parsing 😃
In general you did excellent job filling the gaps. I left some comments where I felt some tweaks might help to increase the chance folks will understand the flow quicker. Although, I'm aware that it's extremely hard to get it all right. I think @youknowriad or @mcsf might find some mistakes in my reasoning 😃
| @@ -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. | |||
There was a problem hiding this comment.
to the current
savefunction 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.
There was a problem hiding this comment.
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
| @@ -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. | |||
|
|
|||
| 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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 recommeded 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. | ||
| 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. |
There was a problem hiding this comment.
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. ❤️
| 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. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
Nice, this is very clear here and in alignment with my previous comments 👍
|
I've made some updates to improve clarity and fix a few issues where the described deprecation flow did not match the code. This was mostly related to how |
glendaviesnz
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
Description
How has this been tested?
Screenshots
Types of changes
Checklist: