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

Jump to bottom

Bug/24286 create block error #24287

Merged
merged 10 commits into from
Jul 30, 2020
Merged

Bug/24286 create block error #24287

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
96b832f
Merge remote-tracking branch 'upstream/master'
Mar 19, 2020
e5a20f4
Merge branch 'master' of github.com:ryanwelcher/gutenberg
May 29, 2020
b5d9c57
Merge branch 'master' of github.com:ryanwelcher/gutenberg
Jun 12, 2020
f2caf37
Merge branch 'master' of github.com:ryanwelcher/gutenberg
Jul 30, 2020
4843ff7
Add warning to createBlock.
Jul 30, 2020
937de91
Add tests for new change.
Jul 30, 2020
b1dad82
Update tests to actually work.
Jul 30, 2020
594b9c4
Add changes to package-lock.json
Jul 30, 2020
d125251
Throw an exception instead of a warning.
Jul 30, 2020
1e68ce0
Update docs.
Jul 30, 2020
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
2 changes: 1 addition & 1 deletion packages/blocks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ _Parameters_

_Returns_

- `Object`: Block object.
- `(Object|boolean)`: Block object or false if the block name is not a registered block.

<a name="doBlocksMatchTemplate" href="#doBlocksMatchTemplate">#</a> **doBlocksMatchTemplate**

Expand Down
1 change: 1 addition & 0 deletions packages/blocks/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"@wordpress/icons": "file:../icons",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/shortcode": "file:../shortcode",
"@wordpress/warning": "file:../warning",
"hpq": "^1.3.0",
"lodash": "^4.17.15",
"rememo": "^3.0.0",
Expand Down
8 changes: 7 additions & 1 deletion packages/blocks/src/api/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
* WordPress dependencies
*/
import { createHooks, applyFilters } from '@wordpress/hooks';
import warning from '@wordpress/warning';

/**
* Internal dependencies
Expand All @@ -40,12 +41,17 @@ import { normalizeBlockType } from './utils';
* @param {Object} attributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
*
* @return {Object} Block object.
* @return {(Object|boolean)} Block object or false if the block name is not a registered block.
*/
export function createBlock( name, attributes = {}, innerBlocks = [] ) {
// Get the type definition associated with a registered block.
const blockType = getBlockType( name );

if ( undefined === blockType ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important but yoda conditions are not a guideline in JavaScript because we enforce strict equality.

warning( `Block type '${ name }' is not registered.` );
return false;
Copy link
Contributor

@youknowriad youknowriad Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Making this non-blocking changes the expectation from someone calling this function.

We do things like that in general in Gutenberg configs.map( config => createBlock( config.name, config.attributes, config.innerBlocks )

With the current implementation, this will hide bugs and just move them down the road. IMO, we should throw an exception here instead of calling warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad thanks for the review here! I have updated the PR to throw an exception.

}

// Ensure attributes contains only values defined by block type, and merge
// default values for missing attributes.
const sanitizedAttributes = reduce(
Expand Down
8 changes: 8 additions & 0 deletions packages/blocks/src/api/test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ describe( 'block factory', () => {
} );

describe( 'createBlock()', () => {
it( 'should trigger a warning and return false if the blockType is not registered', () => {
const name = 'not/registered';
const createAttempt = createBlock( name );
expect( console ).toHaveWarnedWith(
`Block type '${ name }' is not registered.`
);
expect( createAttempt ).toBe( false );
} );
it( 'should create a block given its blockType, attributes, inner blocks', () => {
registerBlockType( 'core/test-block', {
attributes: {
Expand Down