Fallback to regular subscribe if the store doesn't exist in useSelect#27466
Conversation
kevin940726
added
[Type] Bug
|
Size Change: +6 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
The question here becomes: what happens to a |
Yeah, that makes sense! I'll try to fix this tomorrow. |
|
👋🏻 I'd like to share the findings of my investigation today: We first thought that the problem was related to a specific store not being correctly registered, but it's a more general problem. It lies in the fact that I don't know why in some WP envs some stores might not be available before they're used. As Kai points out in the description, there's some prior art that confirms this behaviour might have been out there for a while. This guard prevents that component from breaking due to the unexisting store, but ends up causing this issue later because of the call to This fix does seem to work well, and might be a suitable one for now. I wanted to further trigger the discussion, however, as there seems to be a sympton of a deeper issue here, for that, I have two questions:
This might not pertain to Gutenberg itself, as the stores that caused the issue so far were registered by third-party lib. I'd like to know your thoughts about that, nevertheless. Thanks! 🙇🏻 |
One thought that comes to my mind is that some JS code might execute before other code. For example, say I have two different WordPress plugins, both of which register block editor scripts. My one plugin contains a line which |
|
I'd say it's either, the registration didn't happen yet (timing issues) or the registration happened on a separate |
| const state = useSelect( | ||
| ( select ) => ( { | ||
| count1: select( 'store-1' ).getCounter(), | ||
| blank: select( 'non-existing-store' )?.getCounter(), |
There was a problem hiding this comment.
Can we show a console.warn when selecting a non-existing store? Similar to as we do when registering blocks with invalid category. This way we'd also immediately see that a store is not available (whatever the reason), which could help to track issues that it might cause.
There was a problem hiding this comment.
This has been brought up internally, c.c. @youknowriad.
The counter point is that sometimes it's intended, as shown in the example in the PR description. I'm not sure if it's an anti-pattern or not, but this pattern does exist. I think it needs further discussion and more usage examples.
There was a problem hiding this comment.
Ah, gotcha. Maybe we could consider adding sth like isStoreRegistered to @wordpress/data then. This does sound like a topic for another issue though 👍
|
For now, I try to fallback to regular |
kevin940726
changed the title
I did some debugging there and it seems to be exactly what happened in our case, @noahtallen - a |
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Are we losing performance or we're just in the error range? Thanks for the tests.
kevin940726
force-pushed
the
update/swallow-error-when-subscribe-to-non-exisiting-store
branch
from
cd2f37e to
da91d42
Compare
|
I just rebased master, I don't think we're losing performance now, thx for the check! |
kevin940726
deleted the
update/swallow-error-when-subscribe-to-non-exisiting-store
branch
5 participants
Description
Fix an error when selecting a non-existing store in
useSelectthrow an error. A regression introduced in #26724. This pattern can be seen in some existing code before:https://github.com/Automattic/wp-calypso/blob/9e0e3a8fdc49088aa005cf53c2ebf85a24f0ede4/apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nux/src/wpcom-nux.js#L29
How has this been tested?
Added new test, run
npm run test-unit.Types of changes
Bug fix
Checklist: