Interactivity: Ensure stores are initialized on client #59842
Merged
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
sirreal
added
[Type] Bug
|
Size Change: +18 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
sirreal
force-pushed
the
fix/interactivity-delayed-init-stores
branch
from
d0b586b to
41e1946
Compare
|
I pushed a version of this with the application changes reverted to demonstrate the issue in the tests. That run is available here.. I've updated this branch to include the changes and the tests should pass now. |
sirreal
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad,
ajitbohra,
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. |
DAreRodz
reviewed
Mar 13, 2024
packages/interactivity/src/init.js
Outdated
Comment on lines
42
to
58
| for ( const node of nodes ) { | ||
| // Before initializing the vdom, make sure a store exists for the namespace. | ||
| // This ensures that directives can subscribe to the store even if it has | ||
| // not yet been created on the client so that directives can be updated when | ||
| // stores are later created. | ||
| let namespace = /** @type {HTMLElement} */ ( node ).dataset[ | ||
| `${ directivePrefix }Interactive` | ||
| ]; | ||
| try { | ||
| namespace = JSON.parse( namespace ).namespace; | ||
| } catch {} | ||
| if ( ! stores.has( namespace ) ) { | ||
| store( namespace, undefined, { | ||
| lock: universalUnlock, | ||
| } ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting approach. 😄
I was thinking more of checking if stores are defined when directives are evaluated. This is the place:
gutenberg/packages/interactivity/src/hooks.tsx
Lines 260 to 268 in acd79ef
We could―before doing stores.get( namespace )―check if that namespace exists, and create the store at that moment if it doesn't. That would also cover the case where directives are using values with explicit namespaces that could not appear in any data-wp-interactive, e.g.:
<div data-wp-interactive="myblock">
<!-- namespace not in data-wp-interactive -->
<span data-wp-text="myblock/extra::state.text"></span>
</div>Let me know what you think, @sirreal 🙂
There was a problem hiding this comment.
That would also cover the case where directives are using values with explicit namespaces that could not appear in any data-wp-interactive
That's interesting… I hadn't considered that there can be namespaces not associated with a data-wp-interactive directive 🤔
In that case it would be better to do this somewhere else, probably resolve as you suggest. I've pushed changes to revert my approach and ensure stores exist in resolve.
sirreal
force-pushed
the
fix/interactivity-delayed-init-stores
branch
from
d23c292 to
9395e19
Compare
|
@DAreRodz do you think this should be backported to 6.5? |
@sirreal, let me think. This PR fixes directive subscription to reactive state props when two conditions are met:
I'm not sure how usual this specific case would be, at least for this first version. This is a small bug fix; it's tested, so for me, it would be safe to include it in 6.5. But I'd say it's not such a big deal if it doesn't make it. 🤔 cc: @gziolo |
|
I’m adding it to 6.5 project board in the triage column so @youknowriad and @getdave can decide about the next steps. |
sirreal
added
the
Backport to WP 6.7 Beta/RC
|
Thanks both, I also added the |
youknowriad
pushed a commit
that referenced
this pull request
Mar 18, 2024
If a store call happens after interactivity has hydrated, subscriptions may not be made correctly which means that when a store is later added, the subscriptions do not exist and the client will not update. This is undesirable. We fix this by creating empty stores during directive resolution, ensuring subscriptions can be made so that subsequent store changes will update the client.
|
I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: c0b82c4 |
youknowriad
added
Backported to WP Core
getdave
pushed a commit
that referenced
this pull request
Mar 18, 2024
If a store call happens after interactivity has hydrated, subscriptions may not be made correctly which means that when a store is later added, the subscriptions do not exist and the client will not update. This is undesirable. We fix this by creating empty stores during directive resolution, ensuring subscriptions can be made so that subsequent store changes will update the client.
|
@sirreal This is on the WP 6.5 Editor board in the "Needs Core Commit" column. However, it seems it would have been included in the packages release. Just checking there's nothing I missed here and there are not outstanding PHP files that require manual backporting. |
|
That's correct, this should be covered by a package release and subsequent package update in core. No other changes to core should be required. Thanks! |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
If a store call happens after interactivity has hydrated, subscriptions may not be made correctly which means that when a store is later added, the subscriptions do not exist and the client will not update. This is undesirable. We fix this by creating empty stores during directive resolution, ensuring subscriptions can be made so that subsequent store changes will update the client.
|
Thanks, I've created #61359 to address flakiness. |
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?
Ensure the client has an interactivity store initialized for every namespace before hydrating.
Why?
If a
storecall happens after interactivity has hydrated, subscriptions may not be made correctly which means that when a store is later added, the subscriptions do not exist and the client will not update. This is undesirable.How?
When performing initialization on the client, before hydrating, a store is created for every namespace that has not already created a store. A store may be created already from server data or by
store()calls on the client, in which case this initialization is skipped.Testing Instructions
This includes e2e tests.