Ensure Query Loop Block queryId Uniqueness
#58804
Draft
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
There are multiple cases which can cause `useInstanceId()` to return IDs that are already in use. Since this function returns IDs based on the number of given blocks present, we will just set the `queryId` to the `instanceId` on every component mounting. This approach is a little bit unstable but it's good enough for the usage.
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @scrobbleme. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
ObliviousHarmony
added
[Type] Bug
|
Size Change: -2 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
tjcafferkey
reviewed
Feb 9, 2024
| }, [ queryId, instanceId ] ); | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setAttributes( { queryId: instanceId } ); | ||
| }, [] ); |
There was a problem hiding this comment.
Do we not want to add instanceId as a dependency to this useEffect hook?
There was a problem hiding this comment.
Initially I didn't @tjcafferkey because this value is persistent during component re-mounting. It seemed reasonable to just declare that the hook should only run on mounting. Adding the instanceId would make the linter happier though so I've gone ahead and added it.
11 tasks
|
Noting that this also addresses the specific report of #55823 but as mentioned in the linked issue comments there is still a deeper need for "private" or non-duplicatable attributes on blocks. |
|
Another |
|
Block duplication feels like a strong candidate for having some mechanism for manipulating it. Either through the use of a filter or perhaps something even as far as a new function you can set on registration. We already have transform handlers there to transform blocks between types so a duplication handler feels really appropriate. |
|
It looks like there's a bug when using the block from this PR in a synced pattern. It causes the editor the freeze and I'll have to dig into this first. |
|
I've chased this down to the re-insertion of sync pattern blocks after the pattern has changed:
This was previously fine because the |
|
Coming back to this, in WooCommerce, I took the route of treating sync patterns as shared. If you put multiple instances of the same pattern on a page, all of them will paginate together. This feels like the right thing to do since the use of a sync pattern explicitly indicates that you want all of the instances of a given block to be identical. I would be curious to get some input about how this block should behave in Gutenberg. How would I go about gathering feedback for a change like this since it goes beyond just fixing the documented bug? It's worth keeping in mind that, actually, this is no different than the current behavior! Query Loop blocks placed in a pattern will share their ID! |
|
I took another look at this pull request and it seems that the approach taken in my Woo PR does not work with the latest version of Gutenberg. I believe this has something to do with partial sync support. It feels like partial sync is 100% the way to solve this problem with sync patterns and so I'll take another look as that feature develops. |
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?
This PR makes sure that the
queryIdfor the Query Loop Block is always unique.Closes #56902
Why?
There are multiple cases which can cause
useInstanceId()to return IDs that are already in use:queryIdto be duplicated as well.instanceIddepends on the number of instances of a given block on a page. This means you can get collisions out of it if you aren't careful about how you delete Query Loop blocks and add more.How?
This approach relies on the fact that the
queryIddoesn't actually have to be stable, it just has to be recorded in the block's attributes so that it can be consumed. As long as it is unique and does not change during frontend page reloads it will work just fine.useInstanceId()is relatively stable since it is just a counter for the number of instances of a given block on a page. This means we can safely and reliably setqueryIdon component mounting. The block won't be marked dirty because typically this value would never change unless the output of theuseInstanceId()hook is altered due to block removal, addition, or moving.Testing Instructions
queryIdwith the code editor.Note: Switching to the code editor and back will trigger new blocks to be inserted and increment the counter. If you refresh the page with the block editor open they will re-index from 0.
Testing Instructions for Keyboard
I'm fine writing this but looking up all of the hotkeys to switch between the code editor and duplicate blocks sounds like a lot 😄