Blocks: Include pre-filter settings in block type registration deprecation filter applications. #16518

epiqueras · 2019-07-10T17:30:50Z

Fixes https://meta.trac.wordpress.org/ticket/4597

Description

PR #16348 created errors for plugins that have block type registration filters in which they expect properties not present in a deprecation entry.

A specific example of this is:

https://github.com/Automattic/jetpack/blob/4c63a49fc0139c3296248e09ee69dedb537e024f/extensions/blocks/videopress/editor

Which expects settings.edit to exist and throws an error that hangs the editor on a white screen.

How has this been tested?

Tests for the new functionality were added in the block type registration tests.

Test suites were ran and they all passed.

Types of Changes

Bug Fix: Include pre-filter settings in block type registration deprecation filter applications to avoid uncaught errors.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

aduth

We should probably update the corresponding test case to assert on the updated behavior.

gutenberg/packages/blocks/src/api/test/registration.js

Lines 341 to 375 in 5756b29

    
           it( 'should apply the blocks.registerBlockType filter to each of the deprecated settings as well as the main block settings', () => { 
        
           	const blockSettingsWithDeprecations = { 
        
           		...defaultBlockSettings, 
        
           		deprecated: [ 
        
           			{ 
        
           				save() { 
        
           					return 1; 
        
           				}, 
        
           			}, 
        
           			{ 
        
           				save() { 
        
           					return 2; 
        
           				}, 
        
           			}, 
        
           		], 
        
           	}; 
        
           	addFilter( 'blocks.registerBlockType', 'core/blocks/without-title', ( settings ) => { 
        
           		return { 
        
           			...settings, 
        
           			attributes: { 
        
           				...settings.attributes, 
        
           				id: { 
        
           					type: 'string', 
        
           				}, 
        
           			}, 
        
           		}; 
        
           	} ); 
        
           	const block = registerBlockType( 'my-plugin/fancy-block-13', blockSettingsWithDeprecations ); 
        
           	expect( block.attributes.id ).toEqual( { type: 'string' } ); 
        
           	expect( block.deprecated[ 0 ].attributes.id ).toEqual( { type: 'string' } ); 
        
           	expect( block.deprecated[ 1 ].attributes.id ).toEqual( { type: 'string' } ); 
        
           } );

aduth · 2019-07-10T17:36:02Z

packages/blocks/src/api/constants.js

We should include a JSDoc describing the purpose of this value.

aduth · 2019-07-10T17:51:00Z

Related (at least partly fixes): #16502

aduth · 2019-07-10T18:02:52Z

Maybe relevant to update here, another consequence of #16348. We previously assumed it was okay (or at least expected) to mutate settings passed through these filters:

gutenberg/packages/block-editor/src/hooks/custom-class-name.js

Lines 34 to 41 in 8995efd

    
           if ( hasBlockSupport( settings, 'customClassName', true ) ) { 
        
           	// Use Lodash's assign to gracefully handle if attributes are undefined 
        
           	settings.attributes = assign( settings.attributes, { 
        
           		className: { 
        
           			type: 'string', 
        
           		}, 
        
           	} ); 
        
           }

I expect might be problematic for multiple runs over the same settings, though I guess if we're merging the objects (creating a new reference) before calling the filter on the deprecated entries, we might not have an issue? Wondering if it would still be safer to create a shallow clone within our own implementation, rather than mutate directly (or, conversely, our own implementation would help us assure mutations work as expected).

Observed as an error when trying to load the editor in Firefox (in Gutenberg 6.1, only when Jetpack activated):

epiqueras · 2019-07-10T19:31:29Z

We should probably update the corresponding test case to assert on the updated behavior.

afea425 Blocks: Test new deprecation filter logic.

We should include a JSDoc describing the purpose of this value.

96b621a Blocks: Add JSDoc to new deprecation entry keys constant.

I expect might be problematic for multiple runs over the same settings, though I guess if we're merging the objects (creating a new reference) before calling the filter on the deprecated entries, we might not have an issue?

Yeah this wasn't a problem during calls for deprecations, but the main call could still mutate it and pass down mutations to the subsequent calls for deprecations. It's fixed now.

108327e Blocks: Shallow copy settings passed to block type registration filter to avoid mutations in subsequent calls for deprecations.

Wondering if it would still be safer to create a shallow clone within our own implementation, rather than mutate directly (or, conversely, our own implementation would help us assure mutations work as expected).

I think we should avoid mutations going forward, but they should not cause any issues and we can't guarantee plugins won't do it.

Observed as an error when trying to load the editor in Firefox (in Gutenberg 6.1, only when Jetpack activated):...

I don't think that's because of this PR.

aduth

I was never able to reproduce the original issue, but the changes seem sensible and I encounter no issues with block registration when running the branch. I might also suggest a look from @talldan to confirm, ideally targeting a patch release tomorrow.

aduth · 2019-07-10T19:34:21Z

packages/blocks/src/api/registration.js

Between this and the shallow clone above, there's some added overhead in the registration of each block. I don't think it will have a substantial impact, but noting since it impacts page load. We could potentially optimize this to skip the Array#map (and also the preFilterSettings assignment above) if ! hasFilter( 'blocks.registerBlockType' ).

That said, now that I write this, I realize since the editor has some default filtering, it will always be true. Perhaps in the future if we refactor away from those filters, we could get some wins here.

Perhaps in the future if we refactor away from those filters, we could get some wins here.

That would be nice and would help in other places too probably.

aduth · 2019-07-10T19:35:22Z

packages/blocks/src/api/registration.js

It could maybe do for an inline comment explaining the purpose of the merge, omit, pick.

aduth · 2019-07-10T19:44:31Z

Observed as an error when trying to load the editor in Firefox (in Gutenberg 6.1, only when Jetpack activated):...

I don't think that's because of this PR.

No, in fact, I'd venture to guess the changes here would resolve the error.

epiqueras · 2019-07-10T19:51:30Z

0ae97ab Blocks: Add inline comments for new deprecation filter logic.

aduth · 2019-07-10T20:26:42Z

The tests appear to be failing.

…ation filter applications.

…r to avoid mutations in subsequent calls for deprecations.

…ult block settings.

epiqueras · 2019-07-10T20:49:28Z

The tests appear to be failing.
@aduth

Yeah, master has new default block settings.

Updated:

7facfea Blocks: Update tests for new deprecation filter logic to use new default block settings.

talldan

Thank you for spotting and fixing the error @epiqueras 🙇

I was also unable to reproduce the original issue, so I haven't been able to validate the fix. Any reproduction steps for the issue?

Was the core of the issue that Jetpack's withVideoPressEdit function had an undefined CoreVideoEdit argument?
https://github.com/Automattic/jetpack/blob/4c63a49fc0139c3296248e09ee69dedb537e024f/extensions/blocks/videopress/edit.js#L20

That does make me wonder whether error handling might also be valuable with this filter (and probably other parts of the api). Even if it's just to throw a slightly more informative error. Worth getting this fix in first though, and then that can be debated.

epiqueras · 2019-07-11T13:53:45Z

@talldan

You need to run Gutenberg 6.1 with the latest Jetpack Blocks changes and I don't think the version in the plugin directory has them.

Was the core of the issue that Jetpack's withVideoPressEdit function had an undefined CoreVideoEdit argument?

The videopress/editor filter was wrapping settings.edit in an HOC, but it was undefined for deprecations so things like accessing .displayName failed.

That does make me wonder whether error handling might also be valuable with this filter (and probably other parts of the api). Even if it's just to throw a slightly more informative error. Worth getting this fix in first though, and then that can be debated.

Yeah, we could wrap the main filter application and each deprecation application with their own try/catch. @youknowriad What do you think?

youknowriad · 2019-07-11T14:54:17Z

Yeah, we could wrap the main filter application and each deprecation application with their own try/catch. @youknowriad What do you think?

Extensibility APIs are in general good candidate for error boundaries/handling.
With Slots, it's easier because we know that the impact is essentially on the UI and won't affect the other parts of the application. For the filters, I think it's a bit more trickier, we can't just ignore the error. I feel the user should be aware of the error somehow even if we can try to "make it work" without the filter.

epiqueras · 2019-07-11T15:15:41Z

True, an entire plugin could break if one of their filters was not applied.

vindl · 2019-07-11T21:14:54Z

I managed to reproduce this with latest Jetpack master and Gutenberg 6.1.0, and verified that this PR resolves the issue.

…ation filter applications. (#16518) * Blocks: Include pre-filter settings in block type registration deprecation filter applications. * Blocks: Test new deprecation filter logic. * Blocks: Add JSDoc to new deprecation entry keys constant. * Blocks: Shallow copy settings passed to block type registration filter to avoid mutations in subsequent calls for deprecations. * Blocks: Add inline comments for new deprecation filter logic. * Blocks: Update tests for new deprecation filter logic to use new default block settings.

* Blocks: Include pre-filter settings in block type registration deprecation filter applications. (#16518) * Blocks: Include pre-filter settings in block type registration deprecation filter applications. * Blocks: Test new deprecation filter logic. * Blocks: Add JSDoc to new deprecation entry keys constant. * Blocks: Shallow copy settings passed to block type registration filter to avoid mutations in subsequent calls for deprecations. * Blocks: Add inline comments for new deprecation filter logic. * Blocks: Update tests for new deprecation filter logic to use new default block settings. * Edit Widgets: Don't run Customizer setup on every preview refresh. (#16544)

epiqueras added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. labels Jul 10, 2019

epiqueras added this to the Gutenberg 6.1 milestone Jul 10, 2019

epiqueras requested review from aduth, ellatrix, gziolo and youknowriad as code owners July 10, 2019 17:30

epiqueras self-assigned this Jul 10, 2019

This was referenced Jul 10, 2019

i can see only white screen after updating gutenberg 6.1 #16502

Closed

Page totally white after installing 6.1.0 #16517

Closed

aduth reviewed Jul 10, 2019

View reviewed changes

aduth requested a review from talldan July 10, 2019 19:36

aduth approved these changes Jul 10, 2019

View reviewed changes

epiqueras added 5 commits July 10, 2019 15:37

Blocks: Include pre-filter settings in block type registration deprec…

4ef0df5

…ation filter applications.

Blocks: Test new deprecation filter logic.

81ee796

Blocks: Add JSDoc to new deprecation entry keys constant.

2784037

Blocks: Shallow copy settings passed to block type registration filte…

ad9443b

…r to avoid mutations in subsequent calls for deprecations.

Blocks: Add inline comments for new deprecation filter logic.

57e4623

epiqueras force-pushed the fix/errors-in-deprecation-filter-applications branch from 0ae97ab to 57e4623 Compare July 10, 2019 20:37

Blocks: Update tests for new deprecation filter logic to use new defa…

7facfea

…ult block settings.

talldan approved these changes Jul 11, 2019

View reviewed changes

youknowriad mentioned this pull request Jul 11, 2019

Gutenberg: Attempt a v6.1.1 update [3] Automattic/wp-calypso#34386

Closed

10 tasks

epiqueras merged commit cb718ff into master Jul 11, 2019

epiqueras deleted the fix/errors-in-deprecation-filter-applications branch July 11, 2019 22:33

VR51 mentioned this pull request Jul 12, 2019

GB 6.1.0 Gives White Screen When Adding Posts or Pages #16553

Closed

dericleeyy mentioned this pull request Jul 12, 2019

Tiled Gallery block disappears with Gutenberg 6.1.0 Automattic/jetpack#13037

Closed

youknowriad mentioned this pull request Jul 12, 2019

Backport/6.1.1 fixes #16562

Merged

bobbingwide mentioned this pull request Jul 14, 2019

Support for WordPress 5.0 and the new block editor bobbingwide/oik-testimonials#3

Closed

jsnajdr mentioned this pull request Jul 2, 2023

Remove serverSideBlockDefinitions from a test #52215

Merged

	it( 'should apply the blocks.registerBlockType filter to each of the deprecated settings as well as the main block settings', () => {
	const blockSettingsWithDeprecations = {
	...defaultBlockSettings,
	deprecated: [
	{
	save() {
	return 1;
	},
	},
	{
	save() {
	return 2;
	},
	},
	],
	};

	addFilter( 'blocks.registerBlockType', 'core/blocks/without-title', ( settings ) => {
	return {
	...settings,
	attributes: {
	...settings.attributes,
	id: {
	type: 'string',
	},
	},
	};
	} );

	const block = registerBlockType( 'my-plugin/fancy-block-13', blockSettingsWithDeprecations );

	expect( block.attributes.id ).toEqual( { type: 'string' } );
	expect( block.deprecated[ 0 ].attributes.id ).toEqual( { type: 'string' } );
	expect( block.deprecated[ 1 ].attributes.id ).toEqual( { type: 'string' } );
	} );

Blocks: Include pre-filter settings in block type registration deprecation filter applications. #16518

Blocks: Include pre-filter settings in block type registration deprecation filter applications. #16518

Uh oh!

Conversation

epiqueras commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Types of Changes

Checklist:

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 10, 2019

Uh oh!

aduth commented Jul 10, 2019

Uh oh!

epiqueras commented Jul 10, 2019

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

aduth Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 10, 2019

Uh oh!

epiqueras commented Jul 10, 2019

Uh oh!

aduth commented Jul 10, 2019

Uh oh!

epiqueras commented Jul 10, 2019

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Jul 11, 2019

Uh oh!

youknowriad commented Jul 11, 2019

Uh oh!

epiqueras commented Jul 11, 2019

Uh oh!

vindl commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

epiqueras commented Jul 10, 2019 •

edited

Loading