-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reusable Blocks: Change from store name provided as a string to store definition #27094
Reusable Blocks: Change from store name provided as a string to store definition #27094
Conversation
/** | ||
* Internal dependencies | ||
*/ | ||
import { store as reusableBlocksStore } from './index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surprising to see a cyclic dependency between store/index.js
and store/controls.js
. Would it be possible to access somehow dispatch
from the current store instead? /cc @adamziel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gziolo The control needs to dispatch data on its own store so it's not easily avoidable. I think the only way of avoiding circular dependency would be not refactoring this file and just leaving the core/reusable-blocks
string as it was before, or perhaps using the same constant as store/index.js
, only imported from e.g. store/constants.js
. This is going to be a general theme in plenty of registry actions and selectors too - we should address all those cases in the same way and either accept the circular dependency (it shouldn't break anything in this case) or use string to refer to the store from the same package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, reusing the constant (internal to the local store's implementation) sounds like a good way of handling this particular case 👍
It looks like we can now remove the following statements from some packages: import '@wordpress/reusable-blocks'; @adamziel, is there anything I could miss that justifies those imports after the refactoring proposed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is good, we can always open a follow-up to address the usage in controls.
Congratulations on your first merged pull request, @grzim! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Description
Regarding #27088
Change from store name provided as a string to store definition applied
How has this been tested?
Not tested yet
Types of changes
Code refactor (non-breaking change)
Checklist: