Skip to content
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.

Sign up for GitHub

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

Data: refuse to register an already registered store #49134

Merged
merged 4 commits into from
Mar 20, 2023
Merged

Data: refuse to register an already registered store #49134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c16611a
Data: refuse to register an already registered store
jsnajdr Mar 16, 2023
9dd4139
Create store lazily, console error on duplicate store
jsnajdr Mar 17, 2023
f039c3a
Add changelog entry
jsnajdr Mar 17, 2023
b97cb2f
Add unit test to check double registration behavior
jsnajdr Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/data/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Breaking Changes

- The `registry.register` function will no longer register a store if another instance is registered with the same name.

## 8.6.0 (2023-03-15)

## 8.5.0 (2023-03-01)
Expand Down
28 changes: 20 additions & 8 deletions packages/data/src/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,18 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
/**
* Registers a store instance.
*
* @param {string} name Store registry name.
* @param {Object} store Store instance object (getSelectors, getActions, subscribe).
* @param {string} name Store registry name.
* @param {Function} createStore Function that creates a store object (getSelectors, getActions, subscribe).
*/
function registerStoreInstance( name, store ) {
function registerStoreInstance( name, createStore ) {
if ( stores[ name ] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a change is worth a CHANGELOG entry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ✅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is worth a unit test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, added a unit test that checks that after a second registration the old data are still there.

// eslint-disable-next-line no-console
console.error( 'Store "' + name + '" is already registered.' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an info or warning call instead? IMO, we want this to be a supported option for stores

return stores[ name ];
}

const store = createStore();

if ( typeof store.getSelectors !== 'function' ) {
throw new TypeError( 'store.getSelectors must be a function' );
}
Expand Down Expand Up @@ -262,6 +270,8 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
// ignore it.
}
}

return store;
}

/**
Expand All @@ -270,15 +280,17 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
* @param {StoreDescriptor} store Store descriptor.
*/
function register( store ) {
Copy link
Member

@noahtallen noahtallen Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

registerStoreInstance( store.name, store.instantiate( registry ) );
registerStoreInstance( store.name, () =>
store.instantiate( registry )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea for the lazy init 👍

);
}

function registerGenericStore( name, store ) {
deprecated( 'wp.data.registerGenericStore', {
since: '5.9',
alternative: 'wp.data.register( storeDescriptor )',
} );
registerStoreInstance( name, store );
registerStoreInstance( name, () => store );
}

/**
Expand All @@ -294,10 +306,10 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
throw new TypeError( 'Must specify store reducer' );
}

const store = createReduxStore( storeName, options ).instantiate(
registry
const store = registerStoreInstance( storeName, () =>
createReduxStore( storeName, options ).instantiate( registry )
);
registerStoreInstance( storeName, store );

return store.store;
}

Expand Down
53 changes: 31 additions & 22 deletions packages/data/src/test/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,41 +481,50 @@ describe( 'createRegistry', () => {
} );

describe( 'register', () => {
const store = createReduxStore( 'demo', {
reducer( state = 'OK', action ) {
if ( action.type === 'UPDATE' ) {
return 'UPDATED';
}
return state;
},
actions: {
update: () => ( { type: 'UPDATE' } ),
},
selectors: {
getValue: ( state ) => state,
},
} );

it( 'should work with the store descriptor as param for select', () => {
const store = createReduxStore( 'demo', {
reducer: ( state = 'OK' ) => state,
selectors: {
getValue: ( state ) => state,
},
} );
registry.register( store );

expect( registry.select( store ).getValue() ).toBe( 'OK' );
} );

it( 'should work with the store descriptor as param for dispatch', async () => {
const store = createReduxStore( 'demo', {
reducer( state = 'OK', action ) {
if ( action.type === 'UPDATE' ) {
return 'UPDATED';
}
return state;
},
actions: {
update() {
return { type: 'UPDATE' };
},
},
selectors: {
getValue: ( state ) => state,
},
} );
registry.register( store );

expect( registry.select( store ).getValue() ).toBe( 'OK' );
await registry.dispatch( store ).update();
expect( registry.select( store ).getValue() ).toBe( 'UPDATED' );
} );

it( 'should keep the existing store instance on duplicate registration', async () => {
registry.register( store );

await registry.dispatch( store ).update();
expect( registry.select( store ).getValue() ).toBe( 'UPDATED' );

registry.register( store );

// check that the state hasn't been reset back to `OK`, as a re-registration would do
expect( registry.select( store ).getValue() ).toBe( 'UPDATED' );

expect( console ).toHaveErroredWith(
'Store "demo" is already registered.'
);
} );
} );

describe( 'select', () => {
Expand Down