Data: Build the basic data controls into every store #25362
Conversation
jsnajdr
added
[Type] Code Quality
|
Size Change: -580 B (0%) Total Size: 1.18 MB
ℹ️ View Unchanged
|
|
i'm not sure about this PR tbh, I think the "syncSelect" control is small enough that duplication is probably better than having the ap-fetch dependency there. |
I tend to agree that the pros don't outweight the cons here. It shows, though, that there might be something slightly wrong with the The select/dispatch controls are so close to the data stores so that they could be part of the |
I agree here 💯 cc @nerrad |
|
Sounds very similar to the evolution of resolvers moving into the wp.data package. I'm not opposed to this being built-in to wp.data by default. Built-in controls would also get rid of the gotcha folks encounter with not defining a controls object when registering a store and that resulting in resolvers not working. Would we deprecate the |
That's one of the options: it would be deprecated, and implemented as a thin wrapper around the builtin Of course, we'd need to find a new home for the We could also fix the unfortunate naming of the There's one thing that's not great about moving the controls to the This is where the // import controls with nice names from dedicated package
import { select, dispatch } from '@wordpress/data-controls';
// and use them in my control
export function *myControl() {
yield select( 'core', 'foo' );
yield dispatch( 'core', bar() );
} |
|
Maybe we can export theme as "controls" variable, it's not tree-shakable but we have just three right? |
We intend to register some controls created by `createRegistryControl` as built-ins with every store. In order to do that, we need to break a dependency cycle where registry creation depends on store registration which depends on creating controls which depends on default registry. The solution is to remove the `defaultRegistry` binding, which is there only to satisfy a typechecker anyway, and doesn't have any runtime impact.
Instead of shipping them in a separate `data-controls` package and require the store author to register them, build them in into every store.
The implementation and the unit test have been moved to the `@wordpress/data` package. The `data-controls` package now exposes only legacy aliases.
…rols Updates unit tests of actions (in `block-directory`, `edit-site` and `core-data`) that inspect the internal properties (like `type`) of controls that the action generator yields. A test that verifies if the action _behaves_ correctly wouldn't need to be changed like that.
jsnajdr
force-pushed
the
use/data-controls-in-block-editor
branch
from
66cf456
to
c2afe8b
Compare
jsnajdr
changed the title
jsnajdr
changed the title
jsnajdr
added
[Package] Data
|
I completely reworked this PR by moving the data controls ( I also wrote a brand new unit test for the controls. I didn't like the original The The last commit migrates the |
packages/data-controls/src/index.js
Outdated
| args, | ||
| }; | ||
| } | ||
| export const dispatch = dataControls.dispatch; |
There was a problem hiding this comment.
do you think we should deprecate these?
There was a problem hiding this comment.
Yes, deprecation would discourage folks from adding new usages of data-controls. I guess the data-controls wrappers should use the @wordpress/deprecated helper?
There was a problem hiding this comment.
@youknowriad I updated the data-controls wrappers to warn about deprecation using the @wordpress/deprecated package, in 51f7fc5.
But now I have problems with unit tests that I don't know how to solve: every unit tests uses the jest-console plugin that checks that the code didn't produce any console messages. But the deprecated() function logs the deprecation messages with console.warn(), so many unit tests for code that still uses the deprecated controls now fail.
Is there any established way how to get out of this?
There was a problem hiding this comment.
Yes, we have some asserters to aknowledge that warnings will be thrown. expect( console ).toHaveWarned();
That said, when we introduce deprecations, we do it after refactoring the Core usage of this API. Do we still have existing places where these APIs are used on Gutenberg?
There was a problem hiding this comment.
Do we still have existing places where these APIs are used on Gutenberg?
Yes, the data-controls are still used in many stores. It will take some 5+ PRs to migrate them one by one.
At this moment, it seems to me the best option is to mark the data-controls as deprecated only after that migration is finished. It shouldn't take long after this PR is merged.
There was a problem hiding this comment.
it seems to me the best option is to mark the data-controls as deprecated only after that migration is finished. It shouldn't take long after this PR is merged.
👍
| * | ||
| * @type {WPDataRegistry} | ||
| */ | ||
| selector.registry = defaultRegistry; |
There was a problem hiding this comment.
Is this some useless leftover?
There was a problem hiding this comment.
I described the motivation for this in the commit's commit message:
We intend to register some controls created by
createRegistryControlas built-ins with every store. In order to do that, we need to break a dependency cycle where registry creation depends on store registration which depends on creating controls which depends on default registry.The solution is to remove the
defaultRegistrybinding, which is there only to satisfy a typechecker anyway, and doesn't have any runtime impact.
Another solution would be to break up the factory.js module and put createRegistrySelector and createRegistryControl into separate modules. createRegistrySelector has the offending defaultRegistry dependency, while createRegistryControl is needed to create a store and therefore introduces a cyclic dependency.
However, to make the API really type-safe, we'd need to rethink how we create the selectors and introduce types like UnregisteredRegistrySelector and RegisteredRegistrySelector. Right now, the usage is like:
const selector = createRegistrySelector( ( select ) => ( state ) => { select( 'core' ) /* ... */ } );
// at this moment the selector is in a weird state where it's either unbound or bound to `defaultRegistry`
// it's not entirely certain whether the following call should crash or not and what `value` is supposed to be
const value = selector( state );
// only after this registration is the `selector` registered and has 100% defined behavior
customRegistry.registerStore( 'mystore', { reducer, selectors: { selector } } );
// this second call can have an entirely different result from the first one, although the `selector`
// value is still the same and has the same type. The `selector` properties have been mutated during
// `registerStore`.
const value = selector( state );I.e., the code is not safe and I don't immediately see how introducing types could prevent the unsafe usages.
There was a problem hiding this comment.
I'm not sure i understand everything here but I agree that depending on the default registry should be avoided anyway. The default registry only exists because it was there first. If we'd start over, we'd probably avoid it entirely.
There was a problem hiding this comment.
I'm not sure i understand everything here but I agree that depending on the default registry should be avoided anyway.
The tl;dr is that the default registry reference started introducing dependency cycles 🙂
|
Thanks for the review @youknowriad 👍 I'll check if unit tests are indeed green after the deprecations have been commented out, and then will merge the PR. |
In the
@wordpress/block-editorpackage, replaces the home-grownSELECTcontrol withSYNC_SELECTfrom@wordpress/data-controls.There is one thing in this PR I don't like very much: the
@wordpress/block-editorbecomes indirectly dependent on@wordpress/api-fetch, although it never uses that code. Thecontrolsobject exported from@wordpress/data-controlscontains theAPI_FETCHfield. Although we don't use it, it's there, and it's not even possible to tree-shake it away. It's a field of an exported object, not a standalone export.How bad is that?