Gutenberg: make sure that editor initialization is executed only once

[vindl]

Fixes #27882 and #27993

Changes proposed in this Pull Request

The problem with our current editor implementation is that whenever we navigate away from it (by clicking Close) and then navigate back, all of the initialization logic will be executed again. That means that API middleware will be applied twice, core blocks registration will be attempted again (producing warnings) etc.

I discussed this with @Automattic/team-calypso in #27882 and @jsnajdr proposed two options:

1. placing the init in the default export function of client/gutenberg/editor/index.js.
2. placing the init in the post controller wrapped with lodash/once.

After experimenting a bit with both approaches, I decided to go with option 2. It seems more readable and easier to reason about to me, because all of the setup logic and sequencing is in one place. We also don't have to pull in redux-bridge for it.

Here is the summary of what has been moved to new initialization function:

• All of the init code from main.jsx - This includes: data plugins registration, core-data init, API middleware setup, Classic Block init, Core Blocks registration, and TinyMCE plugins init.
• The API middleware has been refactored and the utility component has been removed. Previously, this component was ensuring that API middleware was applied before the editor starts firing API requests. However, it was not accounting for the above issue, so same middleware was applied multiple times. I suspect this didn't surface earlier because we are breaking the middleware chain - if that was not the case our mapping would be broken after navigating away and back.
• I moved the editor extension loading (presets) to init function too, since it seems like a better place to contain that functionality. Previously it was placed in edit-post folder, and we'd like to avoid adding Calypso's custom modifications there as much as possible. Maintaining it is hard enough with frequent core changes, and adding our custom code there only makes it harder. We are working to move away from it as soon as possible, but it's not an immediate priority.

Testing instructions

• localStorage.setItem( 'debug', 'calypso:gutenberg' );
• Navigate to http://calypso.localhost:3000/gutenberg/post/
• Notice the editor initialization debug messages in console.
• Click Close.
• Click Back in your browser (at this point there is a similar visual glitch that we observed in Calypsoify redirects, not related to this PR and present in wpcalypso too).
• Verify that editor initialization has not been executed twice by observing the console log. Currently in wpcalypso you'd see repeated core blocks registration warnings.
• Smoke test the editor and verify that this is not causing any regressions.

[matticbot]

Test live: https://calypso.live/?branch=update/gutenberg-initialize-once

[aduth]

once is not applied unique per argument set. The function will be called once ever.

How then do we ensure applyAPIMiddleware( siteSlug ) is called if I edit a post for a second site after I'd already initialized it once before for a first site?

Instead you might want to use a memoization utility capable of handling multiple argument. Lodash's does not. Gutenberg uses (my) memize to this effect.

⇒ n_      
n_ > const initGutenberg = ( userId, siteSlug ) => console.log( 'called with', userId, siteSlug );
undefined
n_ > const initGutenbergWithOnce = _.once( initGutenberg );
undefined
n_ > initGutenbergWithOnce( 1, 'example1.wordpress.com' )
called with 1 example1.wordpress.com
undefined
n_ > initGutenbergWithOnce( 1, 'example2.wordpress.com' )
undefined
n_ > const initGutenbergWithLodashMemoize = _.memoize( initGutenberg );
undefined
n_ > initGutenbergWithLodashMemoize( 1, 'example1.wordpress.com' )
called with 1 example1.wordpress.com
undefined
n_ > initGutenbergWithLodashMemoize( 1, 'example2.wordpress.com' )
undefined
n_ > const initGutenbergWithMemize = require( 'memize' )( initGutenberg );
undefined
n_ > initGutenbergWithMemize( 1, 'example1.wordpress.com' )
called with 1 example1.wordpress.com
undefined
n_ > initGutenbergWithMemize( 1, 'example2.wordpress.com' )
called with 1 example2.wordpress.com
undefined

Or, if the rest of the code is not meant to / cannot be run, save for the API middleware application, maybe it ought to be pulled out and applied separately.

[jsnajdr]

The initGutenberg function is a mix of initialization tasks we really want to do just once per app lifetime (like various requires and module inits) and other tasks we want to do per site or even per editor instance.

The siteSlug here is used to rewrite the URLs of API requests and point them to the per-site WP.com endpoints. once means that these request will be made to the first site visited, forever, even after site switch. We can rewrite that to a callback function:

() => getSelectedSiteSlug( store.getState() );

Then each request will use the the correct site ID, but will still write the results to the singleton core/data store, mixing results from different sites. Not exactly an improvement. Memoizing the init function better won't help either, as long as the data stores remain global singletons.

[vindl]

Good catch @aduth! I haven't considered that while refactoring the initialization.

Or, if the rest of the code is not meant to / cannot be run, save for the API middleware application, maybe it ought to be pulled out and applied separately.

I think it's fine to run the rest only once - for example, we don't want to attempt to register core blocks twice (would generate warnings) etc. So it looks like we should only pull out API middleware application.

[vindl]

Then each request will use the the correct site ID, but will still write the results to the singleton core/data store, mixing results from different sites. Not exactly an improvement. Memoizing the init function better won't help either, as long as the data stores remain global singletons.

@jsnajdr I'm not sure how to resolve this, given the way core data stores and packages are currently working. Do you think that extending this PR by @Copons to reinitialize the editor on each site switch could work? - #29310

[jsnajdr]

I think the combination of the following two changes should work:

1. Reset the core store on each site switch. core is currently not among the resettable stores in Gutenlypso: Reset part of the Gutenberg stores when navigating to the editor #29310 and needs reset only on site switch, not on each editor load.
2. Instead of passing a constant siteSlug to the wpcomPathMappingMiddleware, pass a getSiteSlug callback that returns the latest siteSlug on every call. That makes the middleware map to a different path on a site switch.

If you end up wpcomPathMappingMiddleware, I propose the following API change: instead of this:

wpcomPathMappingMiddleware = ( options, next, getSiteSlug ) => ...
use( partialRight( wpcomPathMappingMiddleware, getSiteSlug ) );

do this:

wpcomPathMappingMiddleware = getSiteSlug => ( options, next ) => ...
use( wpcomPathMappingMiddleware( getSiteSlug ) );

(if you agree it looks like an improvement)

[vindl]

Instead of passing a constant siteSlug to the wpcomPathMappingMiddleware, pass a getSiteSlug callback that returns the latest siteSlug on every call. That makes the middleware map to a different path on a site switch.

Landed this in #29368. I'm going to look into point 1 next.

[vindl]

The first part is resolved in #29445 by @Copons.

[Copons]

should be 😄