Revert conditional selects from #47243 #49953
Closed
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
|
Size Change: -63 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in abde826. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4753535420
|
FWIW, in Woo we are committed to updating the broken usage of the hook. |
Thank you! I created this PR as a stand-by alternative in case the Woo fix was impractical or impossible to ship quickly to everyone affected. |
|
@jsnajdr Thank you for the tag here and all your effort in the previous PR! I somehow missed that the last PR was merged. As I've documented previously in the linked issue, breaking conditional I can't really say if what we're doing is "correct" way to use We'll probably try to upgrade our code base to use the version without this revert sometimes in the future to see if we face any issues. Cheers. |
|
Thanks @kuasha420 for the info on how you use One thing I didn't previously realize is the connection between conditional selects and useSelect( ( select ) => {
const CoreUI = select( CORE_UI );
const CoreUser = select( CORE_USER );
return (
CoreUI.getValue( runKey ) &&
CoreUser.isTourDismissed( tourID ) === false
);
} );Here we always call But what if const isTourDismissed = createRegistrySelector( ( select ) => ( state, slug ) => {
const CoreUser = select( CORE_USER );
const CoreSite = select( CORE_SITE );
dismissedTourSlugs = CoreUser.getDismissedFeatureTourSlugs();
if ( undefined === dismissedTourSlugs ) {
dismissedTourSlugs = CoreSite.getDismissedTourSlugs();
}
return dismissedTourSlugs.includes( slug );
} );Now we're in trouble because it's impossible to make sure that In #47243 (comment) @youknowriad suggested that we just document that conditional selects are not supported. That would work if we had a documented and reliable way how to avoid the problem, but with registry selectors I'm afraid we no longer have this. |
|
Seems there is no longer demand to perform the revert. Closing. |
14 tasks
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.
Proposes to revert #47243 (partially, there were changes like new tests and test refactors that we're keeping) because:
useSelectusage in WooCommerce (Fix issue that breaks the WooCommerce Home Page when Gutenberg 15.5 is active woocommerce/woocommerce#37641). It's Woo's fault that they're using the hook in a really bad way, but anyway.At the same time I'm not very happy about this revert because it brings back incorrect behavior of
useSelect. For example, here's Google Site Kit folks discussing the bad behavior of conditional selects: google/site-kit-wp#6356 (comment) Apparently they've been running into the bug. Maybe @kuasha420 would have some feedback to share.