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

Jump to bottom

Refactor the core/data store to be independent from the registry #14634

Merged
merged 7 commits into from
Mar 28, 2019
Merged

Refactor the core/data store to be independent from the registry #14634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9044dc5
Refactor the core/data store to be independent from the registry
youknowriad Mar 26, 2019
a215c0e
Fix unit tests
youknowriad Mar 26, 2019
b440ce8
Override core inline scripts
youknowriad Mar 26, 2019
43b95fb
Fix monkey-patching the stores
youknowriad Mar 26, 2019
59d43d2
Document the empty subscribe function
youknowriad Mar 27, 2019
7400e01
Destructure metadata
youknowriad Mar 27, 2019
7e6e3af
forEach instead of map
youknowriad Mar 27, 2019
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
1 change: 1 addition & 0 deletions lib/packages-dependencies.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
'wp-data' => array(
'lodash',
'wp-compose',
'wp-deprecated',
'wp-element',
'wp-is-shallow-equal',
'wp-priority-queue',
Expand Down
1 change: 1 addition & 0 deletions packages/data/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"dependencies": {
"@babel/runtime": "^7.3.1",
"@wordpress/compose": "file:../compose",
"@wordpress/deprecated": "file:../deprecated",
"@wordpress/element": "file:../element",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/priority-queue": "file:../priority-queue",
Expand Down
107 changes: 56 additions & 51 deletions packages/data/src/namespace-store.js → packages/data/src/namespace-store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,21 @@ import {
get,
mapValues,
} from 'lodash';
import combineReducers from 'turbo-combine-reducers';

/**
* WordPress dependencies
*/
import createReduxRoutineMiddleware from '@wordpress/redux-routine';

/**
* Internal dependencies
*/
import promise from './promise-middleware';
import createResolversCacheMiddleware from './resolvers-cache-middleware';
import promise from '../promise-middleware';
import createResolversCacheMiddleware from '../resolvers-cache-middleware';
import metadataReducer from './metadata/reducer';
import * as metadataSelectors from './metadata/selectors';
import * as metadataActions from './metadata/actions';

/**
* Creates a namespace object with a store derived from the reducer given.
Expand All @@ -27,16 +36,17 @@ export default function createNamespace( key, options, registry ) {
const reducer = options.reducer;
const store = createReduxStore( key, options, registry );

let selectors, actions, resolvers;
if ( options.actions ) {
actions = mapActions( options.actions, store );
}
if ( options.selectors ) {
selectors = mapSelectors( options.selectors, store, registry );
}
let resolvers;
const actions = mapActions( {
...metadataActions,
Copy link
Member

Choose a reason for hiding this comment

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

An initial thought occurred to me that there's risk for conflict between these metadata actions and a store's own. On further consideration, I don't see it likely to occur, or at least it should be known that these "baseline" actions/selectors exist for every store. Which leads me to wonder: How do we document this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I assume these can't be auto-documented. I'll see if I can add a note in the data module README (where we document registerStore)

Copy link
Member

Choose a reason for hiding this comment

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

yes, I assume these can't be auto-documented.

I'm not so overly concerned about the selectors themselves (though it would be nice), more to just general awareness that this is a behavior to expect, the fact that resolution behavior is included in every store by default.

Copy link
Contributor

@nerrad nerrad Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There's potential side-effects here of consuming code being able to "override" default metadata selectors and actions. I almost wonder if there should be some enforcement here: i.e. checking the incoming actions/selectors and ensuring there are no collisions with the corresponding metadata functions? (or maybe change the order in the constructed array so metadata always overwrites the stores own?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're not confident exposing these selectors and actions yet, we can easily add __internal or __experimental for now, what do you think?

Copy link
Contributor

@nerrad nerrad Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm less concerned with exposing them (I think they should be exposed?) and more concerned with potential conflicts.

I do think it'd be better (at least initially) to do this:

// for actions
const actions = mapActions( {
    ...options.actions,
    ...metadataActions,
} );
// and something similar with selectors reversing the order

That way the metadataActions and metadataSelectors take precedence. Of course you may intentionally have them this way so they can be overridden by the incoming store config but if not, I think this is safer.

...options.actions,
}, store );
let selectors = mapSelectors( {
...mapValues( metadataSelectors, ( selector ) => ( state, ...args ) => selector( state.metadata, ...args ) ),
...mapValues( options.selectors, ( selector ) => ( state, ...args ) => selector( state.root, ...args ) ),
}, store, registry );
if ( options.resolvers ) {
const fulfillment = getCoreDataFulfillment( registry, key );
const result = mapResolvers( options.resolvers, selectors, fulfillment, store );
const result = mapResolvers( options.resolvers, selectors, store );
resolvers = result.resolvers;
selectors = result.selectors;
}
Expand Down Expand Up @@ -84,15 +94,32 @@ export default function createNamespace( key, options, registry ) {
* @return {Object} Newly created redux store.
*/
function createReduxStore( key, options, registry ) {
const middlewares = [
createResolversCacheMiddleware( registry, key ),
promise,
];

if ( options.controls ) {
const normalizedControls = mapValues( options.controls, ( control ) => {
return control.isRegistryControl ? control( registry ) : control;
} );
middlewares.push( createReduxRoutineMiddleware( normalizedControls ) );
}

const enhancers = [
applyMiddleware( createResolversCacheMiddleware( registry, key ), promise ),
applyMiddleware( ...middlewares ),
];
if ( typeof window !== 'undefined' && window.__REDUX_DEVTOOLS_EXTENSION__ ) {
enhancers.push( window.__REDUX_DEVTOOLS_EXTENSION__( { name: key, instanceId: key } ) );
}

const { reducer, initialState } = options;
return createStore( reducer, initialState, flowRight( enhancers ) );
const enhancedReducer = combineReducers( {
metadata: metadataReducer,
root: reducer,
} );

return createStore( enhancedReducer, initialState, flowRight( enhancers ) );
}

/**
Expand Down Expand Up @@ -151,11 +178,15 @@ function mapActions( actions, store ) {
*
* @param {Object} resolvers Resolvers to register.
* @param {Object} selectors The current selectors to be modified.
* @param {Object} fulfillment Fulfillment implementation functions.
* @param {Object} store The redux store to which the resolvers should be mapped.
* @return {Object} An object containing updated selectors and resolvers.
*/
function mapResolvers( resolvers, selectors, fulfillment, store ) {
function mapResolvers( resolvers, selectors, store ) {
const mappedResolvers = mapValues( resolvers, ( resolver ) => {
const { fulfill: resolverFulfill = resolver } = resolver;
return { ...resolver, fulfill: resolverFulfill };
} );
youknowriad marked this conversation as resolved.
Show resolved Hide resolved

const mapSelector = ( selector, selectorName ) => {
const resolver = resolvers[ selectorName ];
if ( ! resolver ) {
Expand All @@ -169,68 +200,42 @@ function mapResolvers( resolvers, selectors, fulfillment, store ) {
return;
}

if ( fulfillment.hasStarted( selectorName, args ) ) {
if ( metadataSelectors.hasStartedResolution( store.getState().metadata, selectorName, args ) ) {
return;
}

fulfillment.start( selectorName, args );
await fulfillment.fulfill( selectorName, ...args );
fulfillment.finish( selectorName, args );
store.dispatch( metadataActions.startResolution( selectorName, args ) );
await fulfillResolver( store, mappedResolvers, selectorName, ...args );
Copy link
Member

@aduth aduth Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I seem to be running into an issue where this doesn't actually wait for the fulfillment to complete before marking the resolution as finished:

Screen Shot 2019-03-29 at 11 58 03 AM

(Note in last three actions, I'd have expected "RECEIVE_ITEMS" to have occurred prior to "FINISH_RESOLUTION")

I tried to consider how to reproduce this outside my branch, but since we wait for most all data to be available before rendering anything, it's hard to recreate even when adding a filter like add_filter( 'block_editor_preload_paths', '__return_empty_array' );. Which has me wondering if it could in-fact be an issue which had already existed prior to this change, and only became evident when performing resolution for data which hadn't been initially requested in the display of the editor (in my case, waiting to resolve getPostType( 'wp_block' ) when saving a reusable block). In my branch, since the resolution completes before the data is available, it results in an error because postType in this assignment resolves to undefined.

I'll try to clean up my local working branch to push later this afternoon, but raising now in case anything comes to mind.

Copy link
Member

@aduth aduth Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I found the issue. I'll have a pull request shortly.

Edit: #14711

store.dispatch( metadataActions.finishResolution( selectorName, args ) );
}

fulfillSelector( ...args );
return selector( ...args );
};
};

const mappedResolvers = mapValues( resolvers, ( resolver ) => {
const { fulfill: resolverFulfill = resolver } = resolver;
return { ...resolver, fulfill: resolverFulfill };
} );

return {
resolvers: mappedResolvers,
selectors: mapValues( selectors, mapSelector ),
};
}

/**
* Bundles up fulfillment functions for resolvers.
* @param {Object} registry Registry reference, for fulfilling via resolvers
* @param {string} key Part of the state shape to register the
* selectors for.
* @return {Object} An object providing fulfillment functions.
*/
function getCoreDataFulfillment( registry, key ) {
const { hasStartedResolution } = registry.select( 'core/data' );
const { startResolution, finishResolution } = registry.dispatch( 'core/data' );

return {
hasStarted: ( ...args ) => hasStartedResolution( key, ...args ),
start: ( ...args ) => startResolution( key, ...args ),
finish: ( ...args ) => finishResolution( key, ...args ),
fulfill: ( ...args ) => fulfillWithRegistry( registry, key, ...args ),
};
}

/**
* Calls a resolver given arguments
*
* @param {Object} registry Registry reference, for fulfilling via resolvers
* @param {string} key Part of the state shape to register the
* selectors for.
* @param {Object} store Store reference, for fulfilling via resolvers
* @param {Object} resolvers Store Resolvers
* @param {string} selectorName Selector name to fulfill.
* @param {Array} args Selector Arguments.
* @param {Array} args Selector Arguments.
*/
async function fulfillWithRegistry( registry, key, selectorName, ...args ) {
const namespace = registry.stores[ key ];
const resolver = get( namespace, [ 'resolvers', selectorName ] );
async function fulfillResolver( store, resolvers, selectorName, ...args ) {
const resolver = get( resolvers, [ selectorName ] );
if ( ! resolver ) {
return;
}

const action = resolver.fulfill( ...args );
if ( action ) {
await namespace.store.dispatch( action );
await store.dispatch( action );
}
}
30 changes: 8 additions & 22 deletions packages/data/src/store/actions.js → ...a/src/namespace-store/metadata/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@
* Returns an action object used in signalling that selector resolution has
* started.
*
* @param {string} reducerKey Registered store reducer key.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
*/
export function startResolution( reducerKey, selectorName, args ) {
export function startResolution( selectorName, args ) {
return {
type: 'START_RESOLUTION',
reducerKey,
selectorName,
args,
};
Expand All @@ -21,16 +19,14 @@ export function startResolution( reducerKey, selectorName, args ) {
* Returns an action object used in signalling that selector resolution has
* completed.
*
* @param {string} reducerKey Registered store reducer key.
* @param {string} selectorName Name of selector for which resolver triggered.
* @param {...*} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
*/
export function finishResolution( reducerKey, selectorName, args ) {
export function finishResolution( selectorName, args ) {
return {
type: 'FINISH_RESOLUTION',
reducerKey,
selectorName,
args,
};
Expand All @@ -39,53 +35,43 @@ export function finishResolution( reducerKey, selectorName, args ) {
/**
* Returns an action object used in signalling that we should invalidate the resolution cache.
*
* @param {string} reducerKey Registered store reducer key.
* @param {string} selectorName Name of selector for which resolver should be invalidated.
* @param {Array} args Arguments to associate for uniqueness.
*
* @return {Object} Action object.
*/
export function invalidateResolution( reducerKey, selectorName, args ) {
export function invalidateResolution( selectorName, args ) {
return {
type: 'INVALIDATE_RESOLUTION',
reducerKey,
selectorName,
args,
};
}

/**
* Returns an action object used in signalling that the resolution cache for a
* given reducerKey should be invalidated.
*
* @param {string} reducerKey Registered store reducer key.
* Returns an action object used in signalling that the resolution
* should be invalidated.
*
* @return {Object} Action object.
*/
export function invalidateResolutionForStore( reducerKey ) {
export function invalidateResolutionForStore() {
return {
type: 'INVALIDATE_RESOLUTION_FOR_STORE',
reducerKey,
};
}

/**
* Returns an action object used in signalling that the resolution cache for a
* given reducerKey and selectorName should be invalidated.
* given selectorName should be invalidated.
*
* @param {string} reducerKey Registered store reducer key.
* @param {string} selectorName Name of selector for which all resolvers should
* be invalidated.
*
* @return {Object} Action object.
*/
export function invalidateResolutionForStoreSelector(
reducerKey,
selectorName
) {
export function invalidateResolutionForStoreSelector( selectorName ) {
return {
type: 'INVALIDATE_RESOLUTION_FOR_STORE_SELECTOR',
reducerKey,
selectorName,
};
}
19 changes: 5 additions & 14 deletions packages/data/src/store/reducer.js → ...a/src/namespace-store/metadata/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@ import { onSubKey } from './utils';
* Reducer function returning next state for selector resolution of
* subkeys, object form:
*
* reducerKey -> selectorName -> EquivalentKeyMap<Array,boolean>
* selectorName -> EquivalentKeyMap<Array,boolean>
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @returns {Object} Next state.
*/
const subKeysIsResolved = flowRight( [
onSubKey( 'reducerKey' ),
onSubKey( 'selectorName' ),
] )( ( state = new EquivalentKeyMap(), action ) => {
switch ( action.type ) {
Expand All @@ -44,7 +43,7 @@ const subKeysIsResolved = flowRight( [
/**
* Reducer function returning next state for selector resolution, object form:
*
* reducerKey -> selectorName -> EquivalentKeyMap<Array, boolean>
* selectorName -> EquivalentKeyMap<Array, boolean>
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
Expand All @@ -54,18 +53,10 @@ const subKeysIsResolved = flowRight( [
const isResolved = ( state = {}, action ) => {
switch ( action.type ) {
case 'INVALIDATE_RESOLUTION_FOR_STORE':
return has( state, action.reducerKey ) ?
omit( state, [ action.reducerKey ] ) :
state;
return {};
case 'INVALIDATE_RESOLUTION_FOR_STORE_SELECTOR':
return has( state, [ action.reducerKey, action.selectorName ] ) ?
{
...state,
[ action.reducerKey ]: omit(
state[ action.reducerKey ],
[ action.selectorName ]
),
} :
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
return has( state, [ action.selectorName ] ) ?
omit( state, [ action.selectorName ] ) :
state;
case 'START_RESOLUTION':
case 'FINISH_RESOLUTION':
Expand Down
Loading