Use @wordpress/warning during block registration instead of console.error and console.warn
#63610
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Feature] Block API
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Member
|
There are 4 failing unit tests reported that needs to be still fixed and this will be good to go. I really like this refactoring 💯 |
SantosGuillamot
commented
Jul 16, 2024
Comment on lines
81
to
95
| it( 'should reject blocks without a namespace', () => { | ||
| const block = registerBlockType( 'doing-it-wrong' ); | ||
| expect( console ).toHaveErroredWith( | ||
| expect( console ).toHaveWarnedWith( | ||
| 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' | ||
| ); | ||
| expect( block ).toBeUndefined(); | ||
| } ); | ||
|
|
||
| it( 'should reject blocks with too many namespaces', () => { | ||
| const block = registerBlockType( 'doing/it/wrong' ); | ||
| expect( console ).toHaveErroredWith( | ||
| expect( console ).toHaveWarnedWith( | ||
| 'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block' | ||
| ); | ||
| expect( block ).toBeUndefined(); | ||
| } ); |
Contributor
Author
There was a problem hiding this comment.
It seems that the @wordpress/warning package only triggers one message, which is causing these tests to fail: link. I assume that's expected, right? Any idea how to solve that?
Before
After
Member
There was a problem hiding this comment.
OK, so the same message is printed only once. You would have to reset it through:
https://github.com/WordPress/gutenberg/blob/release/14.2/packages/warning/src/utils.js
There are multiple ways in Jest to do it, you can import logged and call a method on this set, for example.
Member
There was a problem hiding this comment.
import logged from 'path/to/warning/src/utils.js';
logged.clear();
Member
There was a problem hiding this comment.
gutenberg/packages/warning/src/test/index.js
Lines 10 to 13 in 4171f27
Contributor
Author
There was a problem hiding this comment.
Okay, I made the changes in this commit and tests are passing (at least locally).
However, importing from '../../../../warning/src/utils' feels pretty weird. What happens to other projects wanting to use this package and add tests like ours but don't have the package in the same repo? Should we export it somehow?
Member
There was a problem hiding this comment.
I wouldn’t worry about it. You could also force Jest to reload the package with some of the utils they provide:
|
Size Change: -615 B (-0.04%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
gziolo
approved these changes
Jul 16, 2024
Member
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
That looks great. Thank you for a swift follow-up.
SantosGuillamot
deleted the
update/use-wordpress-warning-in-blocks-registration
branch
64 tasks
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…e.error` and `console.warn` (WordPress#63610) * Use `@wordpress/warning` in block registration * Clear logged set Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: gziolo <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
As suggested here by @gziolo , in this pull request I explore the possibility of using
@wordpress/warningduring block registration instead of the currentconsole.errorandconsole.warncalls.This means that all the previous errors are now warnings, which I am not sure if it's fine:
Before
After
Why?
To unify how these use cases are handled and to benefit from the functionalities the package provides by default.
How?
Just change the console calls with the
warningfunction and adapt the tests.Testing Instructions
Automated tests should pass.