Interactivity API: Prevent empty namespace or different namespaces from killing the runtime #61409
Conversation
cbravobernal
changed the title
cbravobernal
added
[Type] Bug
|
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.
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.
cbravobernal
force-pushed
the
fix/prevent-no-namespace-error
branch
2 times, most recently
from
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.
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.
Is the acc[ key ] access as deep as it can go before erroring sufficient to subscribe?
There was a problem hiding this comment.
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.
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.
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.
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.
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
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.
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.
<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.
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.
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
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.
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.
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. |
cbravobernal
force-pushed
the
fix/prevent-no-namespace-error
branch
from
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.
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.
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.
Removed in f587e3d
|
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 |
ajlende
added
the
[Feature] Interactivity API
|
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 eliminatedThat'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-interactiveis different than the namespace defined instore.jsfile.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-interactivedirective can bediffor 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