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

Gutenberg: Reset core resolvers on site change #29445

Merged
merged 4 commits into from
Dec 17, 2018

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Dec 14, 2018

Changes proposed in this Pull Request

  • Reset Gutenberg's core resolvers on site change in order to avoid accidentally sending REST request to the previously selected site.

To manage to do this, I've introduced:

  • A new gutenberg/calypso store (name can be changed: I don't have any preferences compared to calypso/gutenberg, and even simply calypso) with a single selectedSiteId attribute.
    It keeps track of the most recent selected site ID on the last Gutenberg editor load.
    This is needed so that, when we open Gutenberg again, we can check if meanwhile we changed the site.

  • A more granular way of resetting single Gutenberg stores.
    Before: registry.reset() knew by itself which stores to reset.
    After: registry.reset( 'core/foobar' ), the store name needs to be provided explicitly.

  • A resolver invalidation function inspired by Core Gutenberg (props to @jsnajdr) called when navigating to Gutenberg, if the selected site has changed since last time Gutenberg was loaded.

Context: #28302 (review)

Testing instructions

  • Open Calypso locally and enable the Gutenberg debug: localStorage.debug = 'calypso:gutenberg' (a reload is required). Filter the console for "reset" to remove the noise.
  • Navigate to Gutenberg at /block-editor.
  • Make sure core/editor and core/notices are reset:
> calypso:gutenberg Resetting core/editor store
> calypso:gutenberg Resetting core/notices store
  • Navigate back to the posts list and change site.
  • Navigate back to Gutenberg.
  • Make sure core/editor, core/notices, and the core resolvers are reset:
> calypso:gutenberg Resetting core/editor store
> calypso:gutenberg Resetting core/notices store
> calypso:gutenberg Resetting core store resolvers: getPostType,hasUploadPermissions
  • Navigate back to the posts list and, this time without changing site, open Gutenberg again.
  • Make sure only core/editor and core/notices are reset.

Regression check 1:

  • Create a post with Gutenberg and publish it.
  • Keep on screen the publish sidebar and the success notice, and press ^++H to open the keyboard shortcuts modal.
  • Navigate back to the posts list with the browser's back arrow.
  • Open another Gutenberg.
  • Make sure that the publish sidebar is closed, and the success notice and the modal are hidden.

Regression check 2:

  • With the Gutenberg debug still enabled, filter the console for sending api.
  • Update a post with Gutenberg.
  • Make sure all the console logs report the currently selected site:
calypso:gutenberg Sending API request to: /wp/v2/sites/first-site.wordpress.com/posts/123
  • Navigate back to the posts list and change site.
  • Clear the console.
  • Update a post with Gutenberg on the new site.
  • Make sure all the console logs report the currently selected site:
calypso:gutenberg Sending API request to: /wp/v2/sites/second-site.wordpress.com/posts/456

Fixes #29369

@Copons Copons added [Type] Bug When a feature is broken and / or not performing as intended [Pri] BLOCKER Requires immediate attention. [Goal] Gutenberg Working towards full integration with Gutenberg labels Dec 14, 2018
@Copons Copons self-assigned this Dec 14, 2018
@matticbot
Copy link
Contributor

@@ -34,26 +34,24 @@ 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

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

@Copons Copons force-pushed the update/29369-gutenberg-reset-core-data-on-site-switch branch from 5d12168 to dbdad45 Compare December 17, 2018 14:40
@Copons Copons changed the title Gutenberg: Reset core/data store on site change Gutenberg: Reset core/data resolvers on site change Dec 17, 2018
@Copons Copons changed the title Gutenberg: Reset core/data resolvers on site change Gutenberg: Reset core resolvers on site change Dec 17, 2018
@Copons Copons requested a review from a team December 17, 2018 15:16
@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 17, 2018
@Copons Copons requested a review from aduth December 17, 2018 15:16
Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

This tests well for me. Good work @Copons!

The code also looks good to me, but I'd prefer if someone else can confirm it since I'm not feeling confidence enough yet with the data layer (especially with the complexity we're handling here) and I might miss something :)

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Tests well for me! :shipit:

@vindl vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 17, 2018
// 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. :)

@Copons Copons merged commit 6897360 into master Dec 17, 2018
@Copons Copons deleted the update/29369-gutenberg-reset-core-data-on-site-switch branch December 17, 2018 16:24
blowery added a commit that referenced this pull request Dec 18, 2018
…sh-2019

* origin/master:
  change "expert" to "support" in page route and title (#29459)
  Add note to concierge upsell page about sessions only being offered in English. (#29461)
  Jetpack Blocks: Fix webpack warnings due to dynamic import (#29509)
  Gutenberg: Reset core resolvers on site change (#29445)
  Signup: Remove Masterbar from Signup (#28886)
  Fix missing bumpStat call (#29504)
  Gutenberg Jetpack Preset: Generate imports dynamically from index.json (#29435)
  Fix the clean:public script to do a better job cleaning public/ folder (#29354)
  Tiled gallery: Add alignWide support (#29493)
  Tiled Gallery: Add noResize to block save (#29496)
  Show G Suite user fields by default (#29458)
  ColorThemes: Add GA and bumpStat events for scheme picking (#29413)
  Remove legacy mock for extensions reducer (#29397)
  Antispam promo card: tweak copy to make it clearer (#29440)
  prevent 0 as street number for ebanx checkouts (#29487)
  Gutenberg: Update Related Posts to use the posts endpoint (#29439)
  remove override on payment methods name in India (#29406)
  Add a space to separate "the" from the holiday name placeholder. (#29479)
  Revert "Migrate my-sites/sharing to webpack css pipeline (#28607)" (#29463)
  Gutenpack Subscription Block (Take two) (#28887)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg [Pri] BLOCKER Requires immediate attention. [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenlypso: flush stores when site is switched
6 participants