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: update packages to latest versions #27778

Merged
merged 7 commits into from
Oct 16, 2018
+266 −224
Merged

Gutenberg: update packages to latest versions #27778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd76eab
Gutenberg: update packages to latest versions
vindl Oct 10, 2018
ed8c75e
Add postLock mock to get the editor to boot
vindl Oct 11, 2018
d34ba9c
Fix test errors in block-serialization-spec-parser
vindl Oct 11, 2018
ac207a6
Try removing the jQuery external
vindl Oct 12, 2018
fdbc969
Register data plugins
vindl Oct 15, 2018
bf78286
Ensure that @wordpress/core-data store is created only after plugin r…
jsnajdr Oct 16, 2018
06bcef3
Remove superfluous userId check
vindl Oct 16, 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
18 changes: 17 additions & 1 deletion client/gutenberg/editor/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
* External dependencies
*/
import React from 'react';
import { plugins, use } from '@wordpress/data';
import { has, uniqueId } from 'lodash';

/**
* Internal dependencies
*/
import GutenbergEditor from 'gutenberg/editor/main';
import { getCurrentUserId } from 'state/current-user/selectors';
import { getSelectedSiteId } from 'state/ui/selectors';

function determinePostType( context ) {
Expand All @@ -32,6 +33,15 @@ function getPostID( context ) {
return parseInt( context.params.post, 10 );
}

// Trying to follow the initialization steps from https://github.com/WordPress/gutenberg/blob/de2fab7b8d66eea6c1aeb4a51308d47225fc5df8/lib/client-assets.php#L260
function registerDataPlugins( userId ) {
const storageKey = 'WP_DATA_USER_' + userId;

use( plugins.persistence, { storageKey: storageKey } );
use( plugins.asyncGenerator );
use( plugins.controls );
}

export const post = ( context, next ) => {
//see post-editor/controller.js for reference

Expand All @@ -43,13 +53,19 @@ export const post = ( context, next ) => {
const unsubscribe = context.store.subscribe( () => {
const state = context.store.getState();
const siteId = getSelectedSiteId( state );
const userId = getCurrentUserId( state );

if ( ! siteId ) {
return;
}

unsubscribe();

registerDataPlugins( userId );
Copy link
Member

Choose a reason for hiding this comment

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

The trouble is that this plugin registration call comes too late, at a time when the @wordpress/core-data store is already created and registered. Without the appropriate middlewares.

The @wordpress/core-data store is created during the static initialization of the module, i.e., when the import statement

import '@wordpress/core-data';

is evaluated.

That happens earlier in this file when importing the gutenberg/editor/main module. That module imports @wordpress/core-data and even if it didn't, importing anything from @wordpress/editor will recursively import (an initialize) core-data.

The only way to escape the static initialization is to import the gutenberg/editor/main using a require() call after calling registerDataPlugins. That's what I did in bf78286 and it seems to have fixed things.

It would be better (for Calypso at least) if @wordpress/core-data exported an initialization function that we can call at our convenience instead of initializing statically. We can only register the plugins once we know the userId (for the persistence plugin) and that's not available yet when modules are statically evaluated. We need to wait at least for the Redux store initialization.

Adding the plugins should fix it in theory 🤷‍♂️ Maybe we're adding them too late?

That was my suspicion too, tried moving them before core-data store initialization but no luck so far.

How exactly did you try to do that? Apparently I was more lucky when trying the same thing 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

How exactly did you try to do that? Apparently I was more lucky when trying the same thing 🙂

I tried moving the registration call around to get it to execute before import '@wordpress/core-data'; and added some logging, but completely missed the fact that the above will always be executed first during static initialization. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the fix and detailed explanation Jarda! 🙇

Copy link
Member

Choose a reason for hiding this comment

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

Just one more thing: registerDataPlugins will be called every time we enter a Gutenberg route. Open editor, navigate elsewhere, open editor again -- two calls.

The same thing happens with the registerCoreBlocks and disableTips calls when mounting the GutenbergEditor React component.

All these calls should only ever happen once.

Copy link
Member Author

@vindl vindl Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is partly a pre-existing problem we've decided to start a follow up issue and handle it in a separate PR: #27882


// Avoids initializing core-data store before data package plugins are registered in registerDataPlugins.
const GutenbergEditor = require( 'gutenberg/editor/main' ).default;

context.primary = (
<GutenbergEditor { ...{ siteId, postId, postType, uniqueDraftKey, isDemoContent } } />
);
Expand Down
1 change: 1 addition & 0 deletions client/gutenberg/editor/main.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class GutenbergEditor extends Component {
autosaveInterval: 3, //interval to debounce autosaving events, in seconds.
titlePlaceholder: translate( 'Add title' ),
bodyPlaceholder: translate( 'Write your story' ),
postLock: {},
};

return (
Expand Down
12 changes: 12 additions & 0 deletions client/gutenberg/editor/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`block-serialization-spec-parser parse() works properly 1`] = `
Array [
Object {
"attrs": Object {},
"blockName": "core/more",
"innerBlocks": Array [],
"innerHTML": "<!--more-->",
},
]
`;
11 changes: 1 addition & 10 deletions client/gutenberg/editor/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,16 +158,7 @@ describe( 'block-serialization-spec-parser', () => {
test( 'parse() works properly', () => {
const result = parse( '<!-- wp:core/more --><!--more--><!-- /wp:core/more -->' );

expect( result ).toMatchInlineSnapshot( `
Array [
Object {
"attrs": null,
"blockName": "core/more",
"innerBlocks": Array [],
"innerHTML": "<!--more-->",
},
]
` );
expect( result ).toMatchSnapshot();
} );
} );

Expand Down
Loading