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

Block versioning: Introduce a migration function #3673

Merged
merged 2 commits into from
Jan 19, 2018
Merged

Block versioning: Introduce a migration function #3673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c79bbcb
Block versioning: Introduce a migration function
youknowriad Nov 27, 2017
3ea6f2b
Adding a comment explaining how the deprecated versions work
youknowriad Jan 19, 2018
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
14 changes: 10 additions & 4 deletions blocks/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,16 @@ export function getAttributesFromDeprecatedVersion( blockType, innerHTML, attrib
...omit( blockType, [ 'attributes', 'save', 'supports' ] ), // Parsing/Serialization properties
...blockType.deprecated[ i ],
};
const deprecatedBlockAttributes = getBlockAttributes( deprecatedBlockType, innerHTML, attributes );
const isValid = isValidBlock( innerHTML, deprecatedBlockType, deprecatedBlockAttributes );
if ( isValid ) {
return deprecatedBlockAttributes;

try {
Copy link
Member

Choose a reason for hiding this comment

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

This could use some more comments explaining the process.

const deprecatedBlockAttributes = getBlockAttributes( deprecatedBlockType, innerHTML, attributes );
const migratedBlockAttributes = deprecatedBlockType.migrate ? deprecatedBlockType.migrate( deprecatedBlockAttributes ) : deprecatedBlockAttributes;
const isValid = isValidBlock( innerHTML, deprecatedBlockType, deprecatedBlockAttributes );
if ( isValid ) {
return migratedBlockAttributes;
}
} catch ( error ) {
// ignore error, it means this deprecated version is invalid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning here is that the migrate function is meant to work properly only when it produces a valid block which means we should just ignore errors.

Copy link
Member

Choose a reason for hiding this comment

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

Where is the error being thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no error nowhere. The idea is that migrate function could access non-existing attributes in invalid blocks. For example:

migrate = ( oldAttributes ) => ( { newAttributeKey: oldAttributes.key1.key2 } )

On an invalid block, this could throw an error since oldAttributese.key1 could be undefined for instance while on valid blocks it can be mandatory for example. The idea is that the migrate function is meant to work only for valid blocks.

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 think even the parsing could produce errors in theory

Copy link
Member

Choose a reason for hiding this comment

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

On an invalid block, this could throw an error since oldAttributese.key1 could be undefined for instance while on valid blocks it can be mandatory for example. The idea is that the migrate function is meant to work only for valid blocks.

I feel a bit uneasy about the fact that we anticipate that this will occur 😬

}
}
}
Expand Down
3 changes: 2 additions & 1 deletion blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ describe( 'block parser', () => {
},
},
save: ( { attributes } ) => <span>{ attributes.fruit }</span>,
migrate: ( attributes ) => ( { fruit: 'Big ' + attributes.fruit } ),
},
],
} );
Expand All @@ -314,7 +315,7 @@ describe( 'block parser', () => {
{ fruit: 'Bananas' }
);
expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( { fruit: 'Bananas' } );
expect( block.attributes ).toEqual( { fruit: 'Big Bananas' } );
expect( block.isValid ).toBe( true );
expectFailingBlockValidation();
} );
Expand Down