Interactivity API: Improve data-wp-context debugging by validating it as a stringified JSON Object. #61045
Conversation
This file contains 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
cbravobernal
added
the
[Feature] Interactivity API
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @cbravobernal! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
Size Change: -1.65 kB (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
cbravobernal
requested review from
spacedmonkey,
luisherranz and
DAreRodz
as code owners
|
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. |
cbravobernal
added
[Type] Code Quality
sirreal
requested changes
Apr 26, 2024
Comment on lines
250
to
252
| // translators: %s: store namespace. | ||
| __( | ||
| 'The value of data-wp-context in "%s" store must be a valid stringified JSON object.' |
There was a problem hiding this comment.
I don't know whether translation data would be extracted from this package, but because it's a module and nothing about modules adds the translation data when they're enqueued (or in the importmap), I don't think localization will work here.
Since this is developer-facing and we don't have a good solution, it seems fine to use the untranslated text.
There was a problem hiding this comment.
Let's remove it. i18n would need a module version too.
| // No change should be made if `defaultEntry` does not exist. | ||
| const contextStack = useMemo( () => { | ||
| if ( defaultEntry ) { | ||
| const { namespace, value } = defaultEntry; | ||
| // Check that the value is a JSON object. Send a console warning if not. | ||
| if ( SCRIPT_DEBUG && typeof value !== 'object' ) { |
There was a problem hiding this comment.
null and [] would both pass this check, but I think the intention is to allow "plain" objects.
We have this function in a couple places already in iAPI:
gutenberg/packages/interactivity/src/store.ts
Lines 19 to 20 in 2b1b80e
gutenberg/packages/interactivity/src/directives.js
Lines 25 to 26 in 2b1b80e
One of those is in this file, let's use it.
There was a problem hiding this comment.
Oh, I completely missed it. Thanks!
cbravobernal
changed the title
cbravobernal
force-pushed
the
enhancement/improve-data-wp-context-debug
branch
from
0596b1e
to
af69f1c
Compare
sirreal
reviewed
Apr 26, 2024
Comment on lines
17
to
19
| const { sprintf, __ } = window.wp.i18n; | ||
| const { warning } = window.wp; | ||
|
|
There was a problem hiding this comment.
Suggested change
| const { sprintf, __ } = window.wp.i18n; | |
| const { warning } = window.wp; |
There was a problem hiding this comment.
I thought I've removed it. I guess I messed with the push.
lib/interactivity-api.php
Outdated
Comment on lines
30
to
33
| if ( SCRIPT_DEBUG ) { | ||
| wp_enqueue_script( 'wp-i18n' ); | ||
| wp_enqueue_script( 'wp-warning' ); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if ( SCRIPT_DEBUG ) { | |
| wp_enqueue_script( 'wp-i18n' ); | |
| wp_enqueue_script( 'wp-warning' ); | |
| } |
There was a problem hiding this comment.
I thought I've removed it. I guess I messed with the push.
Comment on lines
247
to
248
| if ( SCRIPT_DEBUG && isPlainObject(value) ) { | ||
| console.warn('The value of data-wp-context in "%s" store must be a valid stringified JSON object.') |
There was a problem hiding this comment.
I think the condition is off, there's a missing %s replacement, and the formatting looks off too. Maybe this would be good?
Suggested change
| if ( SCRIPT_DEBUG && isPlainObject(value) ) { | |
| console.warn('The value of data-wp-context in "%s" store must be a valid stringified JSON object.') | |
| if ( SCRIPT_DEBUG && ! isPlainObject( value ) ) { | |
| console.warn( 'Context values must be objects, found a value of type "%s" in the "%s" store.', typeof value, namespace ); |
There was a problem hiding this comment.
I thought I've changed it. I guess I messed with the push.
sirreal
approved these changes
Apr 29, 2024
| @@ -241,6 +241,13 @@ export default () => { | |||
| const contextStack = useMemo( () => { | |||
| if ( defaultEntry ) { | |||
| const { namespace, value } = defaultEntry; | |||
| // Check that the value is a JSON object. Send a console warning if not. | |||
| if ( SCRIPT_DEBUG && ! isPlainObject( value ) ) { | |||
There was a problem hiding this comment.
When this is published as a package, we don't want to require these arbitrary global variables to be declared, so we should add a typeof check like is done in the warning package (this may require formatting):
Suggested change
| if ( SCRIPT_DEBUG && ! isPlainObject( value ) ) { | |
| if ( typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true && ! isPlainObject( value ) ) { |
gutenberg/packages/warning/src/index.js
Lines 6 to 8 in 649eddf
Otherwise, when this is used in other environments we'd get a ReferenceError.
I'm a little bit concerned that we don't lint this type of thing 😕
|
Flaky tests detected in 7c34f41. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8880285685
|
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?
Add a console warning if data-wp-context is not a JSON.
Why?
I'm afraid it may be too late to prevent developers from adding any string into
data-wp-context, but we can at least add a warning if the content of the directive is not the expected one.How?
I did on the
directives.jsbut, should we add adoing_it_wrong()notice in the server instead?Testing Instructions
1.) Add an interactive block via
npx @wordpress/create-block@latest counter-block --template @wordpress/create-block-interactive-template2.) Edit the render file to use a non stringified JSON object as
data-wp-context. Something like3.) Add your block to a post or page.
4.) Check the warning on the frontend while running
npm run dev.5.) Check the warning does not appear if it has a production build.
Screenshots or screencast