-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Interactivity API: Prevent empty namespace or different namespaces from killing the runtime #61409
Conversation
Size Change: +678 B (+0.04%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
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. |
packages/interactivity/src/hooks.tsx
Outdated
console.warn( | ||
`The namespace "${ namespace }" defined in "data-wp-interactive" does not match with the store.` | ||
); |
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 think this message is accurate. It means we got an error as we tried to follow the path into the store, it doesn't necessarily have anything to do with the namespace.
I think we can go with a message that says that - there was an error when trying to follow the path, being sure to include the path and the namespace.
4fd660f
to
df4bb57
Compare
packages/interactivity/src/hooks.tsx
Outdated
if ( isDebug ) { | ||
// eslint-disable-next-line no-console | ||
console.warn( | ||
`There was an error when trying to resolve the path "${ path }" in the namespace "${ namespace }".` |
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.
We need to support lazy/dynamically initialized stores. This means that instead of throwing an error, we need to subscribe to the store. This way, when the store is finally loaded, the component will re-render, and everything will work.
I think @DAreRodz was working on it.
To make it as efficient as possible, we need to subscribe as deeply as possible. DeepSignal can subscribe to properties that have not been defined. So, for example, if you are trying to access state.obj.prop and obj is not defined, we should subscribe to state.obj. It shouldn't be complex to do. We just have to let it keep subscribing (which it does automatically) until it throws the error. So basically it's just catching the error.
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.
Is the acc[ key ]
access as deep as it can go before erroring sufficient to subscribe?
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.
Yes. It subscribes to the undefined
value of the key
key. Once that key is replaced with a real value, primitive or object, the computations are invalidated and everything rerenders, resubscribing to the new values (deeper this time if key is an object).
packages/interactivity/src/hooks.tsx
Outdated
if ( isDebug ) { | ||
// eslint-disable-next-line no-console | ||
console.warn( | ||
`Namespace cannot be an empty string. Error found when trying to use "${ path }"` |
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 wonder if there is a use case for an island that wants to hydrate but doesn't have a namespace, and all its directives use the custom namespace syntax.
Anyway, we can add this and then remove it later if necessary 👍🙂
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 worried about that myself and did some testing. This check seems to use the "resolved" namespace, so this warns:
<div data-wp-interactive data-wp-text='state.x'></div>
But this is fine:
<div data-wp-interactive data-wp-text='explicitNS::state.x'></div>
@cbravobernal if we don't have a test like this can we add it? It seems perfectly legitimate to have an interactive region without its own namespace.
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 think these won't warn (see #61409 (comment)):
<div data-wp-interactive='null' data-wp-text='state.x'></div>
<div data-wp-interactive='{}' data-wp-text='state.x'></div>
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.
Good call @DAreRodz - I think we should revisit this section to fix some of that - we JSON.parse attribute values too aggressively:
gutenberg/packages/interactivity/src/vdom.ts
Lines 82 to 97 in 78e53bb
let [ ns, value ] = nsPathRegExp | |
.exec( attributes[ i ].value ) | |
?.slice( 1 ) ?? [ null, attributes[ i ].value ]; | |
try { | |
value = JSON.parse( value ); | |
} catch ( e ) {} | |
if ( n === islandAttr ) { | |
island = true; | |
namespaces.push( | |
typeof value === 'string' | |
? value | |
: value?.namespace ?? null | |
); | |
} else { | |
directives.push( [ n, ns, value ] ); | |
} |
Maybe something like this (only for the interactive
directive):
- If the value is falsy => null ns
- If the value starts with
{
(I don't think namespaces are allowed to start with this?)- Try to parse and get the namespace property
- otherwise
null
- Otherwise, the namespace is the string - should we check that it's a valid namespace??
I don't think that any other directives should ever be JSON parsed.
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 think that any other directives should ever be JSON parsed.
What about data-wp-context
?
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.
<div data-wp-interactive='null' data-wp-text='state.x'></div>
<div data-wp-interactive='{}' data-wp-text='state.x'></div>
Now they should.
Maybe something like this (only for the interactive directive)....
Should we do this in a follow-up PR?
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.
Yep, let's leave those refactors for another PR.
What about data-wp-context?
🙃
packages/interactivity/src/hooks.tsx
Outdated
@@ -260,18 +261,35 @@ export const directive = ( | |||
|
|||
// Resolve the path to some property of the store object. | |||
const resolve = ( path, namespace ) => { | |||
if ( namespace === '' ) { |
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 wondering why this strict check, and it looks like when data-wp-interactive
is empty, we don't add null
to the namespace stack but empty strings.
Just follow this logic, considering that value
is an empty string:
gutenberg/packages/interactivity/src/vdom.ts
Lines 85 to 94 in 9293b4b
try { | |
value = JSON.parse( value ); | |
} catch ( e ) {} | |
if ( n === islandAttr ) { | |
island = true; | |
namespaces.push( | |
typeof value === 'string' | |
? value | |
: value?.namespace ?? null | |
); |
As the JSON.parse()
execution fails, value
is still an empty string, the type is string
, so the final namespace we push is ''
.
However, if the runtime has not processed any data-wp-interactive
element, or the value is null
or an empty object, the namespace can actually be null
. In those cases, the check could fail. 😬
I guess we could use null
everywhere to make it consistent when no namespace is defined, right? 🤔
And also we can simply check if namespace
is a falsy value or not.
if ( namespace === '' ) { | |
if ( ! namespace ) { |
cc: @luisherranz, @sirreal
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.
That seems good 👍
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.
If the namespace is falsy, it should not do anything, which means keeping the current namespace. But since we are popping the namespace afterward, the current namespace must be added again, whether null or a valid namespace.
Does that make sense?
By the way, can anyone check how the logic works on the server? It should be consistent in both places.
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.
By the way, can anyone check how the logic works on the server? It should be consistent in both places.
I'll do it.
I refactored it a little and added some tests. Feel free to re-review. |
3df11e5
to
414e15b
Compare
|
||
export const warn = ( message ) => { | ||
// @ts-expect-error | ||
if ( typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true ) { |
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.
Flagging as something I'll need to update in #61486
packages/interactivity/src/hooks.tsx
Outdated
return path.split( '.' ).reduce( ( acc, key ) => acc[ key ], current ); | ||
} catch ( e ) { | ||
warn( | ||
`The path "${ path }" could not be resolved in the "${ namespace }" store.` |
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.
Loading a store after the initial hydration is legit; I don't think we should add a warning that we must remove later. It's going to cause confusion.
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.
Removed in f587e3d
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.
Thanks, Carlos.
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 sans the minor typo I mentioned
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ packages/e2e-tests/plugins/interactive-blocks/namespace/render.php ❔ packages/e2e-tests/plugins/interactive-blocks/namespace/view.asset.php |
Flagging that all warning messages end up being included in the production bundle when using the ![]() ![]() ![]()
|
To add some context, the babel plugin mentioned works because it's going to transform the What we have here is like this: if ( problem ) {
warn( 'oh no' );
}
function warn( msg ) {
if (SCRIPT_DEBUG) { /*…*/ };
} After compilation (replacement and optimization) we get this: if ( problem ) {
warn( 'oh no' );
}
function warn( msg ) {} So the message and the function call are still there, which is what we'd hoped to eliminate in optimization. The babel plugin works by replacing the if (SCRIPT_DEBUG) {
warn( 'oh no' );
};
// optimized - completely eliminated That's why the function call is nice for reducing duplication, but is annoying to optimize away (requiring some more-or-less heavy-handed transformation). One idea I was playing with was terser annotations. If we always annotate the |
I created #61765 to track this. |
What?
Similar to #61249, the Interactivity API throws an error in these two cases:
data-wp-interactive
is different than the namespace defined instore.js
file.Why?
We don't want that interactive blocks with these errors make the other ones stop working.
How?
Add some checks and try-catchs.
Testing Instructions
Without the PR. Add an interactive block, the
data-wp-interactive
directive can bediff
or an empty string.render.php
view.js
The second button should not log. Some errors should appear.
With the PR.
The second button should log. Some warnings should appear.
Testing Instructions for Keyboard
Screenshots or screencast