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.

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

Introduce the concept of registry selectors #13662

Merged
merged 2 commits into from
Feb 6, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
21 changes: 4 additions & 17 deletions packages/core-data/src/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { map, find, get, filter, compact, defaultTo } from 'lodash';
/**
* WordPress dependencies
*/
import { select } from '@wordpress/data';
import { createRegistrySelector } from '@wordpress/data';
import deprecated from '@wordpress/deprecated';

/**
Expand All @@ -16,19 +16,6 @@ import deprecated from '@wordpress/deprecated';
import { REDUCER_KEY } from './name';
import { getQueriedItems } from './queried-data';

/**
* Returns true if resolution is in progress for the core selector of the given
* name and arguments.
*
* @param {string} selectorName Core data selector name.
* @param {...*} args Arguments passed to selector.
*
* @return {boolean} Whether resolution is in progress.
*/
function isResolving( selectorName, ...args ) {
return select( 'core/data' ).isResolving( REDUCER_KEY, selectorName, args );
}

/**
* Returns true if a request is in progress for embed preview data, or false
* otherwise.
Expand All @@ -38,9 +25,9 @@ function isResolving( selectorName, ...args ) {
*
* @return {boolean} Whether a request is in progress for an embed preview.
*/
export function isRequestingEmbedPreview( state, url ) {
return isResolving( 'getEmbedPreview', url );
}
export const isRequestingEmbedPreview = createRegistrySelector( ( registry ) => ( state, url ) => {
return registry.select( 'core/data' ).isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
} );

/**
* Returns all available authors.
Expand Down
12 changes: 12 additions & 0 deletions packages/data/src/factory.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Mark a function as a registry selector.
*
* @param {function} registrySelector Function receiving a registry object and returning a state selector.
*
* @return {function} marked registry selector.
*/
export function createRegistrySelector( registrySelector ) {
registrySelector.isRegistrySelector = true;

return registrySelector;
}
1 change: 1 addition & 0 deletions packages/data/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export { default as RegistryProvider, RegistryConsumer } from './components/regi
export { default as __experimentalAsyncModeProvider } from './components/async-mode-provider';
export { createRegistry } from './registry';
export { plugins };
export { createRegistrySelector } from './factory';

/**
* The combineReducers helper function turns an object whose values are different
Expand Down
12 changes: 8 additions & 4 deletions packages/data/src/namespace-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import createResolversCacheMiddleware from './resolvers-cache-middleware';
*
* @param {string} key Identifying string used for namespace and redex dev tools.
* @param {Object} options Contains reducer, actions, selectors, and resolvers.
* @param {Object} registry Temporary registry reference, required for namespace updates.
* @param {Object} registry registry reference.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:

Suggested change
* @param {Object} registry registry reference.
* @param {Object} registry Registry reference.

*
* @return {Object} Store Object.
*/
Expand All @@ -32,7 +32,7 @@ export default function createNamespace( key, options, registry ) {
actions = mapActions( options.actions, store );
}
if ( options.selectors ) {
selectors = mapSelectors( options.selectors, store );
selectors = mapSelectors( options.selectors, store, registry );
}
if ( options.resolvers ) {
const fulfillment = getCoreDataFulfillment( registry, key );
Expand Down Expand Up @@ -100,10 +100,14 @@ function createReduxStore( reducer, key, registry ) {
* public facing API. Selectors will get passed the
* state as first argument.
* @param {Object} store The redux store to which the selectors should be mapped.
* @param {Object} registry registry reference.
*
* @return {Object} Selectors mapped to the redux store provided.
*/
function mapSelectors( selectors, store ) {
const createStateSelector = ( selector ) => function runSelector() {
function mapSelectors( selectors, store, registry ) {
const createStateSelector = ( registeredSelector ) => function runSelector() {
const selector = registeredSelector.isRegistrySelector ? registeredSelector( registry ) : registeredSelector;
Copy link
Member

@aduth aduth Feb 6, 2019

Choose a reason for hiding this comment

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

While the logic here is fairly trivial, runSelector is our hottest path in the application.

Is it possible to assign this once when mapSelectors is first called? The registry should stay constant, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I originally wanted to implement this feature regardless of the store implementation (outside namespace-store) but it means the computation is done on each select so I moved it here to solve the issue but it seems like I didn't :P I just thought I did. I'll follow-up


// This function is an optimized implementation of:
//
// selector( store.getState(), ...arguments )
Expand Down
22 changes: 22 additions & 0 deletions packages/data/src/test/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { castArray, mapValues } from 'lodash';
* Internal dependencies
*/
import { createRegistry } from '../registry';
import { createRegistrySelector } from '../factory';

describe( 'createRegistry', () => {
let registry;
Expand Down Expand Up @@ -441,6 +442,27 @@ describe( 'createRegistry', () => {
expect( registry.select( 'reducer1' ).selector2() ).toEqual( 'result2' );
expect( selector2 ).toBeCalledWith( store.getState() );
} );

it( 'should run the registry selectors properly', () => {
const selector1 = () => 'result1';
const selector2 = createRegistrySelector( ( reg ) => () =>
reg.select( 'reducer1' ).selector1()
);
registry.registerStore( 'reducer1', {
reducer: () => 'state1',
selectors: {
selector1,
},
} );
registry.registerStore( 'reducer2', {
reducer: () => 'state1',
selectors: {
selector2,
},
} );

expect( registry.select( 'reducer2' ).selector2() ).toEqual( 'result1' );
} );
} );

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