Skip to content
50 changes: 50 additions & 0 deletions packages/blocks/src/api/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,56 @@ describe( 'blocks', () => {
} );
} );

it( 'should default to empty object when attributes is omitted and without warning', () => {
registerBlockType( 'core/test-block-omitted-attributes', {
title: 'block title',
category: 'text',
save: noop,
} );

// Verify no warning was shown (unlike when explicitly set to null/undefined)
expect( console ).not.toHaveWarned();

const blockType = getBlockType(
'core/test-block-omitted-attributes'
);
expect( blockType.attributes ).toEqual( {} );
} );

it( 'should warn and default to empty object when attributes is undefined', () => {
registerBlockType( 'core/test-block-undefined-attributes', {
title: 'block title',
category: 'text',
save: noop,
attributes: undefined,
} );

expect( console ).toHaveWarnedWith(
'The block "core/test-block-undefined-attributes" must not register attributes as `null` or `undefined`. Use an empty object (`attributes: {}`) or exclude the `attributes` key. In the next Gutenberg release, passing `null` or `undefined` for `attributes` will cause the block editor to crash.'
);

const blockType = getBlockType(
'core/test-block-undefined-attributes'
);
expect( blockType.attributes ).toEqual( {} );
} );

it( 'should warn and default to empty object when attributes is null', () => {
registerBlockType( 'core/test-block-null-attributes', {
title: 'block title',
category: 'text',
save: noop,
attributes: null,
} );

expect( console ).toHaveWarnedWith(
'The block "core/test-block-null-attributes" must not register attributes as `null` or `undefined`. Use an empty object (`attributes: {}`) or exclude the `attributes` key. In the next Gutenberg release, passing `null` or `undefined` for `attributes` will cause the block editor to crash.'
);

const blockType = getBlockType( 'core/test-block-null-attributes' );
expect( blockType.attributes ).toEqual( {} );
} );
Comment thread
jeryj marked this conversation as resolved.
Resolved

it( 'should default to browser-initialized global attributes', () => {
const attributes = { ok: { type: 'boolean' } };
unstable__bootstrapServerSideBlockDefinitions( {
Expand Down
13 changes: 13 additions & 0 deletions packages/blocks/src/store/process-block-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,19 @@ export const processBlockType =
),
};

// Temporary fix to allow block authors time to fix blocks that register attributes as null or undefined.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't have to be temporary. Folks might encounter this in the future, too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with it not being temporary if others agree. I'd defer to folks with more expertise in block registration first.

if (
! blockType.attributes ||
typeof blockType.attributes !== 'object'
) {
warning(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we also include a test for this?

'The block "' +
name +
'" must not register attributes as `null` or `undefined`. Use an empty object (`attributes: {}`) or exclude the `attributes` key. In the next Gutenberg release, passing `null` or `undefined` for `attributes` will cause the block editor to crash.'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, this "in the next release it will crash", isn't accurate, is it? I think we can omit that whole sentence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure. I figured having it be the most aggressive and then relaxing it would be better than starting with a longer timeline.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But it's technically not true, is it? I'd just remove it and keep the warning forever.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But it's technically not true, is it?

I was planning to open a PR that removes it, set a reminder and then get it approved/merged when the time came :)

I'd just remove it and keep the warning forever.

That's fine. I'll just remove it.

);
blockType.attributes = {};
}

const settings = applyFilters(
'blocks.registerBlockType',
blockType,
Expand Down
Loading