Data: refuse to register an already registered store #49134
Conversation
There was a problem hiding this comment.
That change makes sense IMHO.
We are already register()ing stores in core, without checking if they've been registered already:
gutenberg/packages/core-data/src/index.js
Line 66 in 565af6c
That being said, if @wordpress/core-data ends up in two separate bundles that run alongside the same app, this may create similar conflicts to the ones described here. That's undesired behavior and we need to protect against it.
packages/data/src/registry.js
Outdated
| @@ -215,6 +215,10 @@ export function createRegistry( storeConfigs = {}, parent = null ) { | |||
| * @param {Object} store Store instance object (getSelectors, getActions, subscribe). | |||
| */ | |||
| function registerStoreInstance( name, store ) { | |||
| if ( stores[ name ] ) { | |||
| throw new Error( 'duplicate store oh no' ); | |||
There was a problem hiding this comment.
Nit: Maybe we could improve the message a little bit.
There was a problem hiding this comment.
It's going to be replaced by just return; eventually. The error is intentionally bad so that we don't forget about that 🙂
There was a problem hiding this comment.
Sounds good.
We could still do a console.error() though. I think it's something worth informing developers about.
There was a problem hiding this comment.
We could still do a
console.error()though.
That's a good idea, I added a console.error + return instead of throwing. It turns out that registerBlockType also does that.
On the other hand, registerPlugin prints a console error that plugin is already registered, but then proceeds to register the new version. Not sure if we really wanted that.
There was a problem hiding this comment.
Here is an opportunity to define and align the behavior for all those APIs. I saw also @noahtallen commenting in #49134 (comment) that we could use warning from the @wordpress/warning package.
There was a problem hiding this comment.
Yes, I like the idea of using warning in these cases. Let's do that in a separate PR. I'd rather keep this one focused on the data package and its store registration code.
There was a problem hiding this comment.
I think @wordpress/warning is a good idea 👍 And I'd be fine to see that done in a separate PR.
| @@ -215,6 +215,10 @@ export function createRegistry( storeConfigs = {}, parent = null ) { | |||
| * @param {Object} store Store instance object (getSelectors, getActions, subscribe). | |||
| */ | |||
| function registerStoreInstance( name, store ) { | |||
| if ( stores[ name ] ) { | |||
There was a problem hiding this comment.
Such a change is worth a CHANGELOG entry.
|
Size Change: +44 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I'm personally a big fan of this approach -- I think overwriting the store and anything in it isn't a good default behavior for duplicate registration :)
Gutenberg packages mostly avoid this problem because they're almost always imported via dependency extraction. So this is an awesome change for people who use their own custom data stores
The only bit of caution I have about this is the line in the register function:
registerStoreInstance( store.name, store.instantiate( registry ) );Essentially, there would be two different store objects (from the two different bundles importing the store), and store.instantiate would be called on each individually. Will this be a problem? (E.g. will we end up with two store copies with separate state?) It seemed like store.instantiate was doing a lot of work 🤔
Yes and no, because the second instance would be thrown away after detecting that the first instance is already registered. It would a mere inefficiency if there wasn't the persistence plugin that has some side effects. Hypothetically, it could delete the local storage after reading the state from it, or store data periodically. Then two instances are fighting each other. I solved this in the registerStoreInstance( name, instantiate() );it's now registerStoreInstance( name, () => instantiate() );and the instance will be created only after verifying that |
|
I addressed feedback and I believe the PR is in a mergeable state:
One last thought: maybe we want a |
|
Flaky tests detected in 0070411. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4446315177
|
| @@ -270,15 +280,17 @@ export function createRegistry( storeConfigs = {}, parent = null ) { | |||
| * @param {StoreDescriptor} store Store descriptor. | |||
| */ | |||
| function register( store ) { | |||
| registerStoreInstance( store.name, store.instantiate( registry ) ); | |||
| registerStoreInstance( store.name, () => | |||
| store.instantiate( registry ) | |||
| */ | ||
| function registerStoreInstance( name, store ) { | ||
| function registerStoreInstance( name, createStore ) { | ||
| if ( stores[ name ] ) { |
There was a problem hiding this comment.
I think this change is worth a unit test.
There was a problem hiding this comment.
I agree, added a unit test that checks that after a second registration the old data are still there.
| function registerStoreInstance( name, createStore ) { | ||
| if ( stores[ name ] ) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( 'Store "' + name + '" is already registered.' ); |
There was a problem hiding this comment.
Should this be an info or warning call instead? IMO, we want this to be a supported option for stores
| @@ -270,15 +280,17 @@ export function createRegistry( storeConfigs = {}, parent = null ) { | |||
| * @param {StoreDescriptor} store Store descriptor. | |||
| */ | |||
| function register( store ) { | |||
There was a problem hiding this comment.
I possibly mis-understand this! The extra instance from the second bundle wouldn't ever be instantiated. If that's a problem, should we also return the already registered store here? Then, at least the second store instance could get assigned to the one stored in the registry.
I guess that would make the consumer look more like export const store = register( createReduxStore( ... ) ).
Happy to be corrected on this; I don't understand the internals of store registration that well :)
There was a problem hiding this comment.
The register function currently doesn't return anything, and I'm keeping it that way. Would there be anything useful to return? The store descriptor, it's been passed as argument, the caller already has it. Not useful. And the actual instance of the store, it's a private property of the registry, you're supposed to access it solely through the registry.select and registry.dispatch functions.
The @automattic/data-stores package in Calypso uses a pattern with a register function, and you can implement it as follows:
function register() {
const plansStore = createReduxStore( STORE_NAME, ... );
register( plansStore );
return plansStore;
}and then use as:
const PLANS_STORE = Plans.register();
registry.select( PLANS_STORE ).getPlanProduct( ... );The legacy registerStore function returns a store object, but that's all legacy. It's not even the store object returned by storeDescription.instantiate and used by the data registry, but the raw Redux store. It's useful for the use function for enabling plugins like persistence, nothing else.
There was a problem hiding this comment.
you're supposed to access it solely through the registry.select and registry.dispatch functions.
Ah, now I understand :) Given this example:
// store
export const fooStore = createReduxStore( ... );
register( fooStore );
// bundle1
import { fooStore as foo1 } from 'foo-store';
useSelect( select => select( foo1 ).selector() );
// bundle2
import { fooStore as foo2 } from 'foo-store';
useSelect( select => select( foo2 ).selector() );If you were to somehow compare foo1 === foo2, they would be different objects. I was thinking this could be problematic, but I didn't realize that the important part of the store is just saved in the registry and not in the object returned from createReduxStore (and then exported for use.)
But realizing that export const fooStore is only relevant in that it lets the registry identify the store based on the store id, I see it won't be an issue.
gziolo
added
[Type] Breaking Change
|
Sorry for the late response here. Yes, this change makes sense to me and I'm not really worried about he backward compatibility impact. I feel like this is more of a bug fix for folks registering the same store twice. A dev note should be the way to go here. |
jsnajdr
force-pushed
the
update/no-store-double-register
branch
from
0070411 to
b97cb2f
Compare
In Automattic/wp-calypso#74346 (comment) @noahtallen ran into an issue where a data store is registered twice. Because the offending store is defined in a JS module, and registered by a static top-level call:
and because this module is bundled in two webpack bundles, each for a different plugin.
In this PR I'm changing the
registerStoreInstancebehavior to refuse to register a second instance. At this moment I'm throwing an error, but that's just because I want unit and e2e tests to fail as badly as possible if they happen to re-register any store. In the final version there should be just areturnstatement.There are concerns about backward compatibility, but my intuition is it should be a much smaller issue than it looks like. Does anyone really intentionally re-register a store? The only reason would be trying to clear it back to initial state.
Another argument against re-registering is that the
useSelecthook wouldn't react to it anyway:won't see any update and won't trigger any rerender when the
my-storeis swapped for another instance.what do you think @youknowriad?