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

Fix private registry selectors #50985

Closed
wants to merge 2 commits into from
Closed

Fix private registry selectors #50985

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87a9833
Add tests, including one failing test
noisysocks May 26, 2023
452bb37
Clarify failing test
noisysocks May 29, 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
38 changes: 38 additions & 0 deletions packages/data/src/test/privateAPIs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { createRegistry } from '../registry';
import createReduxStore from '../redux-store';
import { unlock } from '../private-apis';
import { createRegistrySelector } from '../factory';

describe( 'Private data APIs', () => {
let registry;
Expand Down Expand Up @@ -164,6 +165,43 @@ describe( 'Private data APIs', () => {
);
expect( subPrivateSelectors.getSecretDiscount() ).toEqual( 800 );
} );

it( 'should support registry selectors', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a passing test for a fix I included in #50643.

const groceryStore = createStore();
const otherStore = createReduxStore( 'other', {
reducer: ( state = {} ) => state,
} );
registry.register( otherStore );
unlock( otherStore ).registerPrivateSelectors( {
getPrice: createRegistrySelector(
( select ) => () => select( groceryStore ).getPublicPrice()
),
} );
const privateSelectors = unlock( registry.select( otherStore ) );
expect( privateSelectors.getPrice() ).toEqual( 1000 );
} );

it( 'should support calling a private registry selector from a public selector', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the failing test that I can't figure out how to fix.

const groceryStore = createStore();
const storeA = createReduxStore( 'a', {
reducer: ( state = {} ) => state,
} );
registry.register( storeA );
const getPrice = createRegistrySelector(
( select ) => () => select( groceryStore ).getPublicPrice()
);
unlock( storeA ).registerPrivateSelectors( {
getPrice,
} );
const storeB = createReduxStore( 'b', {
reducer: ( state = {} ) => state,
selectors: {
getPrice: ( state ) => getPrice( state ),
Copy link
Member

Choose a reason for hiding this comment

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

This test it bad, you can't really expect that it will work.

  • a registry selector must be registered with a store to "activate" it, i.e., to set the selector.registry reference. Because in storeA it's only registered lazily, and never called, it's not activated yet.
  • in storeB you should register it directly, with selectors: { getPrice }. Then the test would work.
  • because the implemenation sets a .registry property on the getPrice object returned from createRegistrySelector, you should really use getPrice just once. And not register it in two stores. Otherwise both usages will be fighting for the selector.registry field and the results can in theory be unpredictable.

Where is the real-world situation where you're running into this kind of issue?

Copy link
Member Author

@noisysocks noisysocks May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • a registry selector must be registered with a store to "activate" it

The private getPrice selector is associated with storeA.

Because in storeA it's only registered lazily, and never called, it's not activated yet.

That's the bug, isn't it? It shouldn't be lazy.

in storeB you should register it directly, with selectors: { getPrice }. Then the test would work.

But then it won't be private?

I shouldn't have named both selectors getPrice in the test. The intention is that they are two different selectors. Also it's not necessary (and confusing) that there are two stores. I'll update the test to fix this.

Where is the real-world situation where you're running into this kind of issue?

In #50857, select( blockEditorStore ).canInsertBlockTypeUnmemoized is using select( blockEditorStore ).getBlockEditingMode which is a^ private registry selector.

^ Well, was a private registry selector. I've added a workaround for now.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying the test. In the previous version, behavior of store B depended on what was registered in store A, and that was fishy 🙂

I cherry-picked the tests into #51051 and fixed it there by pre-binding private selectors on store instantiation.

I had to modify the test so that the private selector registration is done before the store is instantiated by registry.register. Otherwise we don't have a good opportunity to pre-bind selectors.

The registerPrivateSelectors API is weird in the sense that it allows registering selectors i) multiple times, and ii) after the store has been instantianted one or more times, and iii) the late registration affects also all the instances that already exist, it is retroactive!

That should probably also be eventually fixed. We'll get to that sooner or later.

@adamziel suggests this:

createReduxStore( 'a', {
  selectors: lock( { foo }, { privateFoo } )
} );

It's syntactically elegant, but semantically it's nonsense 🙂 To register a private selector, we need to unlock a private API of @wordpress/data, but here we're achieving that by locking.

What we'd like to do is:

  1. Register private selectors inside the createReduxStore call, so that the entire registration is atomic, and createReduxStore returns a store descriptor that is immutable.
  2. Expose private selectors registration only as a private API, and that means having to call unlock somewhere.

One way to satisfy that could be:

unlock( createReduxStore )( 'a', {
  selectors: { foo },
  privateSelectors: { privateFoo },
} );

Are there others?

Copy link
Contributor

@adamziel adamziel May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's syntactically elegant, but semantically it's nonsense 🙂 To register a private selector, we need to unlock a private API of @wordpress/data, but here we're achieving that by locking.

This is just a way of passing the information into createReduxStore. It would have to unlock the selectors object and then process the private selectors.

Exposing a privateSelectors options on an unlocked version of createReduxStore indeed sounds much better. This is exactly the way in which using private arguments with lock()/unlock() is documented 👍 I can't think there are many other ways, too, or at least I couldn't come up with any when I was updating the contributing guidelines.

Copy link
Member Author

Choose a reason for hiding this comment

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

A private privateSelectors option makes complete sense to me.

},
} );
registry.register( storeB );
expect( registry.select( storeB ).getPrice() ).toEqual( 1000 );
} );
} );

describe( 'private actions', () => {
Expand Down