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

Gutenberg: Reset core resolvers on site change #29445

Merged
merged 4 commits into from
Dec 17, 2018
Merged

Gutenberg: Reset core resolvers on site change #29445

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
aa527ac
Improve reset logic granularity
Copons Dec 14, 2018
3d3e7c9
Avoid unnecessary variable
Copons Dec 14, 2018
bf7951e
Create a new Gutenberg store keeping track of the selectedSiteId changes
Copons Dec 14, 2018
dbdad45
Reset core resolvers on site change
Copons Dec 17, 2018
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 client/gutenberg/editor/calypso-store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const setSelectedSiteId = selectedSiteId => ( {
type: 'GUTENLYPSO_SELECTED_SITE_ID_SET',
selectedSiteId,
} );
19 changes: 19 additions & 0 deletions client/gutenberg/editor/calypso-store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* WordPress Dependencies
*/
import { registerStore } from '@wordpress/data';

/**
* Internal dependencies
*/
import reducer from './reducer';
import * as actions from './actions';
import * as selectors from './selectors';

const store = registerStore( 'gutenberg/calypso', {
reducer,
actions,
selectors,
} );

export default store;
10 changes: 10 additions & 0 deletions client/gutenberg/editor/calypso-store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* eslint-disable wpcalypso/import-docblock */
/**
* WordPress dependencies
*/
import { combineReducers } from '@wordpress/data';

const selectedSiteId = ( state = null, action ) =>
'GUTENLYPSO_SELECTED_SITE_ID_SET' === action.type ? action.selectedSiteId : state;

export default combineReducers( { selectedSiteId } );
1 change: 1 addition & 0 deletions client/gutenberg/editor/calypso-store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const getSelectedSiteId = state => state.selectedSiteId;
23 changes: 18 additions & 5 deletions client/gutenberg/editor/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { has, set, uniqueId } from 'lodash';
* WordPress dependencies
*/
import { setLocaleData } from '@wordpress/i18n';
import { dispatch } from '@wordpress/data';
import { dispatch, select } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -110,6 +110,22 @@ export const loadGutenbergBlockAvailability = store => {
} );
};

export const resetGutenbergState = ( registry, selectedSiteId ) => {
// Always reset core/editor, core/notices, and other UI parts
registry.reset( 'core/editor' );
registry.reset( 'core/notices' );
dispatch( 'core/edit-post' ).closePublishSidebar();
dispatch( 'core/edit-post' ).closeModal();

// Only reset core/data on site change
const previousGutenbergSiteId = select( 'gutenberg/calypso' ).getSelectedSiteId();

if ( !! previousGutenbergSiteId && previousGutenbergSiteId !== selectedSiteId ) {
Copy link
Member

Choose a reason for hiding this comment

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

Neat. :)

registry.resetCoreResolvers();
}
dispatch( 'gutenberg/calypso' ).setSelectedSiteId( selectedSiteId );
};

export const redirect = ( { store: { getState } }, next ) => {
const state = getState();
const siteId = getSelectedSiteId( state );
Expand Down Expand Up @@ -140,10 +156,7 @@ export const post = async ( context, next ) => {

const { Editor, registry } = initGutenberg( userId, context.store );

// Reset the Gutenberg state
registry.reset();
dispatch( 'core/edit-post' ).closePublishSidebar();
dispatch( 'core/edit-post' ).closeModal();
resetGutenbergState( registry, siteId );

return props => (
<Editor { ...{ siteId, postId, postType, uniqueDraftKey, isDemoContent, ...props } } />
Expand Down
41 changes: 26 additions & 15 deletions client/gutenberg/editor/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,34 @@ const addResetToRegistry = registry => {
window.gutenbergState = () => mapValues( registry.stores, ( { store } ) => store.getState() );
}

const resettableStores = [ 'core/editor', 'core/notices' ];

const stores = [];
return {
registerStore( namespace, options ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth @jsnajdr
This part here that extends all the stores with a reset method, is not applied to core/data (aka the store in the data package) as well.
It's easily testable by adding a console.log( namespace ) here, and observe that it "traverses" all expected stores, except core/data.
I'm not sure why, and I always get lost in the logic behind the Gutenberg data module.

In other words, this function here allows me to reset core (aka the store in the core-data package) but not core/data.

Copy link
Member

@jsnajdr jsnajdr Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The reason why the core/data ignores the call to reset() is that it's created and registered very early as part of the createRegistry function.

At that time, the registerStore function is not yet extended with the use( addResetToRegistry ) plugin and the registered reducer is not wrapped with the HoR that reacts to the GUTENLYPSO_RESET action.

I don't think that being able to fully reset the core/data store is even wanted. The core/data store is a bit special: it maintains the resolution flag for all other stores' selectors. We only ever want to reset the resolution flags for one store at a time, not for all of them.

Fortunately, some support for resetting core/data state was added in WordPress/gutenberg#10089 and we can use it for our purposes. Inspired by the resolvers cache middleware, we can implement a function that resets all resolution state for one store:

function resetStoreResolutions( registry, reducerKey ) {
  const resolvers = registry.select( 'core/data' ).getCachedResolvers( reducerKey );
  Object.entries( resolvers ).forEach( ( [ selectorName, resolversByArgs ] ) => {
    resolversByArgs.forEach( ( _, args ) => {
      registry.dispatch( 'core/data' ).invalidateResolution( reducerKey, selectorName, args );
    } );
  } );
}

Then we can add a call to this new function to the reset function:

reset() {
  resettableStores.forEach( reducerKey => {
    registry.stores[ reducerKey ].dispatch( { type: 'GUTENLYPSO_RESET' } );
    resetStoreResolutions( registry, reducerKey );
  } );
}

This should reset everything on site change. There is just one race condition bug that remains: if a request for entities from site ID=1 is in flight, and we switch to site ID=2, then the response from site ID=2 will be received into store with site ID=2!

The cache invalidation code in WordPress/gutenberg#10089 probably has a similar bug: it won't invalidate resolution for selector whose request is in flight. There can be "get a list" and "add to list" requests happening at the same time, and if their "issue request from client", "perform request on server" and "receive response on client" are performed in a certain order, the resulting state will miss the just-added entity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created an issue for tracking at WordPress/gutenberg#12984

let store;
if ( -1 === resettableStores.indexOf( namespace ) ) {
store = registry.registerStore( namespace, options );
} else {
store = registry.registerStore( namespace, {
...options,
reducer: ( state, action ) =>
options.reducer( 'GUTENLYPSO_RESET' === action.type ? undefined : state, action ),
} );
}
const store = registry.registerStore( namespace, {
...options,
reducer: ( state, action ) => {
if ( 'GUTENLYPSO_RESET' === action.type && namespace === action.namespace ) {
debug( `Resetting ${ namespace } store` );
return options.reducer( undefined, action );
}
return options.reducer( state, action );
},
} );
stores.push( store );
return store;
},
reset() {
stores.forEach( store => store.dispatch( { type: 'GUTENLYPSO_RESET' } ) );
reset( namespace ) {
stores.forEach( store => store.dispatch( { type: 'GUTENLYPSO_RESET', namespace } ) );
},
resetCoreResolvers() {
// @see https://github.com/WordPress/gutenberg/blob/e1092c0d0b75fe53ab57bc6c4cc9e32cb2e74e40/packages/data/src/resolvers-cache-middleware.js#L14-L34
const resolvers = registry.select( 'core/data' ).getCachedResolvers( 'core' );
debug( `Resetting core store resolvers: ${ Object.keys( resolvers ).toString() }` );
Object.entries( resolvers ).forEach( ( [ selectorName, resolversByArgs ] ) => {
resolversByArgs.forEach( ( value, args ) => {
registry.dispatch( 'core/data' ).invalidateResolution( 'core', selectorName, args );
} );
} );
},
};
};
Expand All @@ -63,12 +71,15 @@ const addResetToRegistry = registry => {
export const initGutenberg = once( ( userId, store ) => {
debug( 'Starting Gutenberg editor initialization...' );

const registry = use( addResetToRegistry );

debug( 'Registering data plugins' );
const storageKey = 'WP_DATA_USER_' + userId;
use( plugins.persistence, { storageKey: storageKey } );
use( plugins.controls );

const registry = use( addResetToRegistry );
debug( 'Initializing gutenberg/calypso store' );
require( 'gutenberg/editor/calypso-store' );

// We need to ensure that core-data is loaded after the data plugins have been registered.
debug( 'Initializing core-data store' );
Expand Down