-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Update/stricter block validation #1345
Update/stricter block validation #1345
Conversation
This is a required method and all tests which use registerBlockType as some form of fixture will need to be updated.
This will enforce the following: - `save` will be a required method when registering a block type. - `edit` will be an optional method, but must be a callable function.
Anyone fancying a look at this? It's regarding #362. |
blocks/api/registration.js
Outdated
@@ -44,6 +44,19 @@ export function registerBlockType( name, settings ) { | |||
); | |||
return; | |||
} | |||
if ( ! settings || | |||
( settings && typeof settings.save !== 'function' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we write this like?
if (
! settings ||
( settings && typeof settings.save !== 'function' )
)
I think this pattern is the one used in the Gutenberg code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: should we use isFunction
from lodash
? It handles a special case for us.
Have amended, @youknowriad. |
blocks/api/registration.js
Outdated
@@ -44,6 +49,19 @@ export function registerBlockType( name, settings ) { | |||
); | |||
return; | |||
} | |||
if ( ! settings || | |||
( settings && ! isFunction( settings.save ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put this test in one line too?
blocks/api/registration.js
Outdated
); | ||
return; | ||
} | ||
if ( settings && 'edit' in settings && ! isFunction( settings.edit ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the settings
test is useless here, because we already checked for its existence above?
Both issues sorted in latest push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks for the updates
@nylen do you have any other thoughts on this. I know you're behind the validation we had in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally seems fine to me. Some notes above. We should also make sure similar validation is in place on the server side, but for now this would be for the render
function.
blocks/api/registration.js
Outdated
@@ -44,6 +49,18 @@ export function registerBlockType( name, settings ) { | |||
); | |||
return; | |||
} | |||
if ( ! settings || ( settings && ! isFunction( settings.save ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The settings &&
check here is redundant - by this point in the conditional, we already know that settings
is set.
blocks/api/registration.js
Outdated
@@ -44,6 +49,18 @@ export function registerBlockType( name, settings ) { | |||
); | |||
return; | |||
} | |||
if ( ! settings || ( settings && ! isFunction( settings.save ) ) ) { | |||
console.error( | |||
'A "save" function must be created within the block settings.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clean up this message a bit:
The "save" property must be specified and must be a valid function.
blocks/api/test/factory.js
Outdated
@@ -10,6 +11,8 @@ import { createBlock, switchToBlockType } from '../factory'; | |||
import { getBlockTypes, unregisterBlockType, setUnknownTypeHandler, registerBlockType } from '../registration'; | |||
|
|||
describe( 'block factory', () => { | |||
const defaultBlockType = { save: noop }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultBlockSettings
would be a better name here and below.
To clarify: this is fine for a separate PR. |
@nylen / @youknowriad - Both changes above have been made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for working on this!
Note that because of this there's a bunch of warnings when running the unit tests. |
Can de |
Apologies - I've been quite tied up for a few days. My own branch has been deleted, not sure about the other one. In answer to your remark regarding warnings, @swissspidy - I'd fixed all of them on the tip of my branch but I suspect due to the lead time more old patterns of use for the registration have crept in. Not sure who's down for fixing them but happy to work on another issue for it if there is one. |
These warnings should really be errors that fail the build, which will prevent them from reoccurring. See #1769. |
…refactor DarkMode improvements
This is a PR for resolution to issue #362.