-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Test: Extracting the block settings object to test the blocks #448
Conversation
import { blockSettings } from '../'; | ||
|
||
describe( 'heading block', () => { | ||
afterEach( () => { |
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.
Guessing after
is probably more appropriate. This is needed since the block registers itself merely by being imported? Which is an unfortunate side effect. Seems like managing block registration lifecycle in tests will be a huge headache with the current setup. Simply forgetting one of these lifecycle handlers could have rippling effects on future tests, which is a maintainability nightmare.
I was wondering if, not unlike ideas proposed in #336, we could create the block registry as simply an instance of a Redux store, similar to global state. This way tests could be guaranteed to be isolated. Unsure yet where this initialization would occur in the app itself.
Alternatively, a more immediate solution might be to move the registerBlock
call out of library
and simply have the block export its settings. This doesn't set as good an example for third-party block implementers though, for which this pattern may or may not be the best.
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.
a more immediate solution might be to move the registerBlock call out of library and simply have the block export its settings. This doesn't set as good an example for third-party block implementers though, for which this pattern may or may not be the best.
That was my first intention, but I decided not to do it because we'll need to export the "slug" as a separate variable too. Maybe we should include the slug in the exported object? What do you think? And like you said, This doesn't set as good an example for third-party block implementers.
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.
Maybe we should include the slug in the exported object? What do you think?
There's not really a strong reason not to. This is the approach @iseulde took with her block API in the single instance prototype (example). My preference could be explained by it feeling natural to have a signature of ( key, options )
but this generally applies better in the case that options
is optional (for us, it's not).
Conversely, most "register" functions in WordPress do follow this signature pattern:
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 was thinking of leaving the register
signature as is and deconstructing before calling the function.
|
||
it( 'should parse the block properly', () => { | ||
const rawContent = '<h4 style="text-align: right;">Chicken</h4>'; | ||
const attrs = query.parse( rawContent, blockSettings.attributes ); |
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.
What of a block would we really care to test? Maybe we could export these individually rather than the entire block settings object?
export const attributes = {
// ...
};
registerBlock( 'core/heading', {
attributes,
// ...
} );
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 don't know, My preference would go to exporting the whole object as a default export and using that in the tests (avoid test specific exports) but this means including the slug in the object.
@@ -5,11 +5,9 @@ import { registerBlock, query, setUnknownTypeHandler } from 'api'; | |||
|
|||
const { html } = query; | |||
|
|||
registerBlock( 'core/freeform', { | |||
const blockSettings = { |
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.
Missing export
here and in most of the other blocks?
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.
Actually, I exported only the one that I tested for now but thinking we export the object as default (including the slug). see discussion here #448 (comment)
I'm not sure I understand the need for this. Do we ever need to test a block's settings before registering it? If not, we can just call |
@nylen Right! this could be the way to go if we keep the |
I suppose I have a slight preference for leaving the |
So to clarify: The choice here is whether we want to set good examples for plugin authors rather than easily test the block settings (without clearing the registry) or the opposite. I don't have a strong preference here, maybe this is minor for now. I'm closing this and we could revisit and maybe introduce a registry in the redux store or something. |
Ideally it's not an extreme either/or decision. |
Yes, but it looks like it's not possible as is. Would probably need the registry we're talking about. |
In this PR, I'm proposing a way to test the block settings (and precisely parsing each block's attribute for now).
I'm just exporting the block settings and using those in the tests (see the heading block).
Maybe this won't be necessary once we implement the schema parsing? or it can serve as a validation to this approach?