Apply default attributes on block migration #53120
Conversation
|
Size Change: +44 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bf5f6a3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5693725917
|
| ...block, | ||
| attributes: migratedAttributes, | ||
| attributes: getBlockAttributes( |
There was a problem hiding this comment.
The problem with using getBlockAttributes here is that migratedBlock.originalContent is still the old block's content, so any blocks sourcing block attributes from the HTML will be incorrect.
Maybe we need a new function to skip those in this step.
There was a problem hiding this comment.
we're trying to source the new attributes here from the migrated block's new save output?
I'm missing something on what the problem is here, but it seems like we should be able to resolve it whatever it is
There was a problem hiding this comment.
this is all really confusing to me, and applyBlockDeprecatedVersions seems like it would benefit from drawing new terms and avoiding conflating names.
migratedBlock.originalContent is still the old block's content, so any blocks sourcing block attributes from the HTML will be incorrect.
there could be a few ways this goes: the block's HTML doesn't contain the thing that the new attribute definitions are looking for, or it does, or it doesn't have the structure those attribute definitions are looking for.
- for
source[src]with<img src="…">we hopefully isundefinedand an invalid parse. - for
img[src]with<img>we get''which is a valid parse but wrong because it provides an empty string instead of anundefinedvalue. - for
p:contentwith<p>This used to match everything.</p>we get'This used to match everything.'which is a valid parse.
after creating migratedBlock we should be sourcing the new attributes from the old HTML, leaving us with missing or empty parameters that should be supplied by defaults if they aren't there. I'm reminded of the list block which at one point wiped out an entire list of save because the deprecation process returned an empty string instead of saying the block was invalid. I think in those cases changing <ul> to <ult> or some other typo was enough to trigger this flow.
we could also be hitting this backwards: HTML that was generated in a newer version of the block than we currently support. again, we could fail to parse structure and result in an empty value that wipes out content on save instead of preserving it.
maybe the most important piece is knowing if the original content was newer or older than the block in the editor, which actually is impossible due to the lack of versioning.
I think that the idea in this function is to re-attempt to parse the original content using the newer attribute definitions, and I think this PR is saying, "if after that re-parse we are missing attributes, supply the defaults." if that's the case it seems like we should call addDefaultAttributes() inside the deprecation loop and not after it, but I'm not sure that's still what we want.
migratedBlocksources attributes from new definitions- it's sent to
validateBlock, which re-serializes the block with the newly-sourced attributes, then saves it, then compares if it matches theoriginalContent. it's it's valid it implies that the right data was there; probably the attribute definitions had a minor update. - if it's not validated it goes to the next deprecation in the list. this implies that loading and saving led to a different outcome; something is probably missing.
- if it is valid we look for a
migratefunction to transform it further. at this point we can read and write the block and get the same thing we started with.
the validateBlock function calls save with missing attributes, so it seems like we need to supply defaults to migratedBlock but also refrain from persisting them until deprecation/migration is over, since the next deprecation in the loop could supply a default that's missing from the current one.
let migratedBlock = {
...block,
attributes: getBlockAttributes(
deprecatedBlockType,
block.originalContent,
parsedAttributes
),
};
const withDefaultAttributes = withDefaultsForMissingAttributes( deprecatedBlockType, migratedBlock );
let [ isValid ] = validateBlock( withDefaultAttributes, deprecatedBlockType );
if ( ! isValid ) {
}
migratedBlock = withDefaultAttributes;
|
This makes sense to me but it would be good to have a review from someone who is more familiar with the code. |
ajlende
force-pushed
the
fix/apply-default-attributes-on-migration
branch
from
bf5f6a3
to
be4c574
Compare
| ); | ||
|
|
||
| return blockAttributes; | ||
| } |
There was a problem hiding this comment.
this feels like some kind of withDefaultsForMissingAttributes function.
also I think that it could make sense to move to a more imperative approach in this function, both for efficiency sake (no need to allocate the extra maps/arrays or break shallow-equality for attributes with all the required values) and from a clarity standpoint, to highlight the fill-if-missing-logic
export function withDefaultsForMissingAttributes( blockTypeOrName, attributes = {} ) {
const blockType = normalizeBlockType( blockTypeOrName );
if ( ! blockType.attributes ) {
return attributes;
}
let allAttributes = null;
for ( const key in blockType.attributes ) {
if ( undefined !== attributes[ key ] ) {
continue;
}
if ( null === allAttributes ) {
allAttributes = { ...attributes };
}
allAttributes[ key ] = blockType.attributes[ key ].default;
}
// Only return a new object if default values were supplied.
return null !== allAttributes
? allAttributes
: attributes;
}There was a problem hiding this comment.
also I think that it could make sense to move to a more imperative approach in this function, both for efficiency sake (no need to allocate the extra maps/arrays or break shallow-equality for attributes with all the required values) and from a clarity standpoint, to highlight the fill-if-missing-logic
Yeah, it was just a quick copy of getBlockAttributes that didn't try to source values from the block content. Looks like everywhere that used to use _.mapValues uses the Object.entries style since #50285. It's easy enough to change here. getBlockAttributes should also probably be changed for better performance at some point too.
| ...block, | ||
| attributes: migratedAttributes, | ||
| attributes: getBlockAttributes( |
There was a problem hiding this comment.
this is all really confusing to me, and applyBlockDeprecatedVersions seems like it would benefit from drawing new terms and avoiding conflating names.
migratedBlock.originalContent is still the old block's content, so any blocks sourcing block attributes from the HTML will be incorrect.
there could be a few ways this goes: the block's HTML doesn't contain the thing that the new attribute definitions are looking for, or it does, or it doesn't have the structure those attribute definitions are looking for.
- for
source[src]with<img src="…">we hopefully isundefinedand an invalid parse. - for
img[src]with<img>we get''which is a valid parse but wrong because it provides an empty string instead of anundefinedvalue. - for
p:contentwith<p>This used to match everything.</p>we get'This used to match everything.'which is a valid parse.
after creating migratedBlock we should be sourcing the new attributes from the old HTML, leaving us with missing or empty parameters that should be supplied by defaults if they aren't there. I'm reminded of the list block which at one point wiped out an entire list of save because the deprecation process returned an empty string instead of saying the block was invalid. I think in those cases changing <ul> to <ult> or some other typo was enough to trigger this flow.
we could also be hitting this backwards: HTML that was generated in a newer version of the block than we currently support. again, we could fail to parse structure and result in an empty value that wipes out content on save instead of preserving it.
maybe the most important piece is knowing if the original content was newer or older than the block in the editor, which actually is impossible due to the lack of versioning.
I think that the idea in this function is to re-attempt to parse the original content using the newer attribute definitions, and I think this PR is saying, "if after that re-parse we are missing attributes, supply the defaults." if that's the case it seems like we should call addDefaultAttributes() inside the deprecation loop and not after it, but I'm not sure that's still what we want.
migratedBlocksources attributes from new definitions- it's sent to
validateBlock, which re-serializes the block with the newly-sourced attributes, then saves it, then compares if it matches theoriginalContent. it's it's valid it implies that the right data was there; probably the attribute definitions had a minor update. - if it's not validated it goes to the next deprecation in the list. this implies that loading and saving led to a different outcome; something is probably missing.
- if it is valid we look for a
migratefunction to transform it further. at this point we can read and write the block and get the same thing we started with.
the validateBlock function calls save with missing attributes, so it seems like we need to supply defaults to migratedBlock but also refrain from persisting them until deprecation/migration is over, since the next deprecation in the loop could supply a default that's missing from the current one.
let migratedBlock = {
...block,
attributes: getBlockAttributes(
deprecatedBlockType,
block.originalContent,
parsedAttributes
),
};
const withDefaultAttributes = withDefaultsForMissingAttributes( deprecatedBlockType, migratedBlock );
let [ isValid ] = validateBlock( withDefaultAttributes, deprecatedBlockType );
if ( ! isValid ) {
}
migratedBlock = withDefaultAttributes;
@dmsnell I feel the same way about a lot of the stuff in the parser.
The important thing is that the new default attributes are passed to I worked on some refactoring this afternoon, but didn't quite finish. I'll keep with it and ping you again when it's ready. |
The refactoring hasn't gone well. Once I started renaming things, I realized that a lot of the JSDoc types were wrong. Some of them looked like they could be replaced by types from DefinitelyTyped or @wordpress/block-serialization-default-parser. Unfortunately I found that those other sources for types were often wrong or incomplete. Best I can do is run through the code line-by-line in a debugger and update the types everywhere to be more correct. Here's a table of some of the more confusing type/variable name mappings. I'll probably keep it updated as I come across more.
|
gziolo
added
the
[Feature] Block API
artemiomorales
requested review from
oandregal
and removed request for
oandregal
What?
Applies block attribute defaults when migrating blocks.
Why?
When I was running a block deprecation where new attributes were added with defaults (Automattic/block-experiments#334), I expected the defaults to be passed into the migration, but they weren't.
See the example below in Testing Instructions.
Attribute added.
Deprecated block.
Actual Migration.
Expected migration. This includes the default
attr2value after the<br/>.How?
Calls
getBlockAttributes(which applies the defaults) to themigratedAttributeswhen applying block deprecated versions.Testing Instructions
Use the SSCCE below or try a block that you know has a deprecation that adds new attributes with defaults.
Sample block with deprecation.
Deprecated block.
Correct migration.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A