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

Add thunk actions to replace rungen and controls #27276

Merged
merged 2 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
80 changes: 76 additions & 4 deletions packages/data/src/redux-store/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { createStore, applyMiddleware } from 'redux';
import { flowRight, get, mapValues } from 'lodash';
import { flowRight, get, mapValues, omit } from 'lodash';
import combineReducers from 'turbo-combine-reducers';
import EquivalentKeyMap from 'equivalent-key-map';

Expand All @@ -17,6 +17,7 @@ import createReduxRoutineMiddleware from '@wordpress/redux-routine';
import { builtinControls } from '../controls';
import promise from '../promise-middleware';
import createResolversCacheMiddleware from '../resolvers-cache-middleware';
import createThunkMiddleware from './thunk-middleware';
import metadataReducer from './metadata/reducer';
import * as metadataSelectors from './metadata/selectors';
import * as metadataActions from './metadata/actions';
Expand Down Expand Up @@ -81,7 +82,32 @@ export default function createReduxStore( key, options ) {
name: key,
instantiate: ( registry ) => {
const reducer = options.reducer;
const store = instantiateReduxStore( key, options, registry );
const thunkArgs = {
registry,
get dispatch() {
return Object.assign(
( action ) => store.dispatch( action ),
getActions()
);
},
get select() {
return Object.assign(
( selector ) =>
selector( store.__unstableOriginalGetState() ),
getSelectors()
);
},
get resolveSelect() {
return getResolveSelectors();
},
};

const store = instantiateReduxStore(
key,
options,
registry,
thunkArgs
);
const resolversCache = createResolversCache();

let resolvers;
Expand All @@ -92,6 +118,7 @@ export default function createReduxStore( key, options ) {
},
store
);

let selectors = mapSelectors(
{
...mapValues(
Expand Down Expand Up @@ -123,6 +150,8 @@ export default function createReduxStore( key, options ) {

const getSelectors = () => selectors;
const getActions = () => actions;
const getResolveSelectors = () =>
mapResolveSelectors( selectors, store );

// We have some modules monkey-patching the store object
// It's wrong to do so but until we refactor all of our effects to controls
Expand Down Expand Up @@ -156,6 +185,7 @@ export default function createReduxStore( key, options ) {
selectors,
resolvers,
getSelectors,
__experimentalGetResolveSelectors: getResolveSelectors,
getActions,
subscribe,
};
Expand All @@ -171,10 +201,10 @@ export default function createReduxStore( key, options ) {
* describing reducer, actions, selectors,
* and resolvers.
* @param {WPDataRegistry} registry Registry reference.
*
* @param {Object} thunkArgs Argument object for the thunk middleware.
* @return {Object} Newly created redux store.
*/
function instantiateReduxStore( key, options, registry ) {
function instantiateReduxStore( key, options, registry, thunkArgs ) {
const controls = {
...options.controls,
...builtinControls,
Expand All @@ -190,6 +220,10 @@ function instantiateReduxStore( key, options, registry ) {
createReduxRoutineMiddleware( normalizedControls ),
];

if ( options.__experimentalUseThunks ) {
middlewares.push( createThunkMiddleware( thunkArgs ) );
}

const enhancers = [ applyMiddleware( ...middlewares ) ];
if (
typeof window !== 'undefined' &&
Expand Down Expand Up @@ -266,6 +300,44 @@ function mapActions( actions, store ) {
return mapValues( actions, createBoundAction );
}

/**
* Maps selectors to functions that return a resolution promise for them
*
* @param {Object} selectors Selectors to map.
* @param {Object} store The redux store the selectors select from.
* @return {Object} Selectors mapped to their resolution functions.
*/
function mapResolveSelectors( selectors, store ) {
return mapValues(
omit( selectors, [
'getIsResolving',
'hasStartedResolution',
'hasFinishedResolution',
'isResolving',
'getCachedResolvers',
] ),
( selector, selectorName ) => ( ...args ) =>
new Promise( ( resolve ) => {
const hasFinished = () =>
selectors.hasFinishedResolution( selectorName, args );
const getResult = () => selector.apply( null, args );

// trigger the selector (to trigger the resolver)
const result = getResult();
if ( hasFinished() ) {
return resolve( result );
}

const unsubscribe = store.subscribe( () => {
if ( hasFinished() ) {
unsubscribe();
resolve( getResult() );
}
} );
} )
);
}

/**
* Returns resolvers with matched selectors for a given namespace.
* Resolvers are side effects invoked once per argument set of a given selector call,
Expand Down
9 changes: 9 additions & 0 deletions packages/data/src/redux-store/thunk-middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default function createThunkMiddleware( args ) {
return () => ( next ) => ( action ) => {
if ( typeof action === 'function' ) {
return action( args );
}

return next( action );
};
}
55 changes: 11 additions & 44 deletions packages/data/src/registry.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/**
* External dependencies
*/
import { omit, without, mapValues, isObject } from 'lodash';
import memize from 'memize';
import { without, mapValues, isObject } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -102,47 +101,6 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
return result;
}

const getResolveSelectors = memize(
Copy link
Member

Choose a reason for hiding this comment

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

Does removing this usage of memize impact performance?

Copy link
Member

Choose a reason for hiding this comment

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

I ran npm run test-performance with and without this branch and didn't see any regression.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Removing memize impacts performance, but I don't think it's measurable in practice.

Without memize, the selectors are mapped (i.e., the object with resolution functions is created) from scratch every time registry.resolveSelect() is called. This is not needed, the mapping can be done only once.

I'm fixing this in #28544 by doing the mapping only once at store initialization.

( selectors ) => {
return mapValues(
omit( selectors, [
'getIsResolving',
'hasStartedResolution',
'hasFinishedResolution',
'isResolving',
'getCachedResolvers',
] ),
( selector, selectorName ) => {
return ( ...args ) => {
return new Promise( ( resolve ) => {
const hasFinished = () =>
selectors.hasFinishedResolution(
selectorName,
args
);
const getResult = () =>
selector.apply( null, args );

// trigger the selector (to trigger the resolver)
const result = getResult();
if ( hasFinished() ) {
return resolve( result );
}

const unsubscribe = subscribe( () => {
if ( hasFinished() ) {
unsubscribe();
resolve( getResult() );
}
} );
} );
};
}
);
},
{ maxSize: 1 }
);

/**
* Given the name of a registered store, returns an object containing the store's
* selectors pre-bound to state so that you only need to supply additional arguments,
Expand All @@ -155,7 +113,16 @@ export function createRegistry( storeConfigs = {}, parent = null ) {
* @return {Object} Each key of the object matches the name of a selector.
*/
function __experimentalResolveSelect( storeNameOrDefinition ) {
return getResolveSelectors( select( storeNameOrDefinition ) );
const storeName = isObject( storeNameOrDefinition )
? storeNameOrDefinition.name
: storeNameOrDefinition;
__experimentalListeningStores.add( storeName );
const store = stores[ storeName ];
if ( store ) {
return store.__experimentalGetResolveSelectors();
}

return parent && parent.__experimentalResolveSelect( storeName );
}

/**
Expand Down