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

Merged
merged 7 commits into from
Oct 16, 2018
Merged

Conversation

vindl
Copy link
Member

@vindl vindl commented Oct 10, 2018

Resolves #27649

Changes proposed in this Pull Request

Updates the Gutenberg npm packages to their latest versions released on 11th of October.

Testing instructions

  1. Navigate to http://calypso.localhost:3000/gutenberg/ and verify that the editor loads.
  2. Smoke test the editor and check for regressions.
  3. Calypso smoke test.

Testing jQuery removal from externals

I discussed this with @blowery and it should be safe to remove the jQuery from externals.

  1. Make sure that there are no import jQuery in the Calypso codebase.
  2. Make sure that we are not causing regressions for parts of Calypso that use jQuery via load-script.
  3. Check bundle sizes.
  4. Check jQuery 3 (editor sub-dep) and jQuery 1 (via load-script) interaction. For example, this can be done by using some functionality that triggers the jQuery 1 load, and then loading the editor shell.

@vindl vindl added [Status] In Progress [Goal] Gutenberg Working towards full integration with Gutenberg labels Oct 10, 2018
@matticbot
Copy link
Contributor

@vindl vindl self-assigned this Oct 10, 2018
@vindl vindl requested a review from a team October 10, 2018 23:10
@ghost
Copy link

ghost commented Oct 10, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@vindl
Copy link
Member Author

vindl commented Oct 10, 2018

Currently running into Uncaught Error: Given action "UPDATE_POST_LOCK", reducer "postLock" returned undefined. To ignore an action, you must explicitly return the previous state. If you want this reducer to hold no value, you can return null instead of undefined. which is preventing the editor from loading.

Update: resolved in 6824a9ade17c24775170fc57ac4d78c1d7dff3fd, the editor shell boots correctly now.

@vindl
Copy link
Member Author

vindl commented Oct 11, 2018

The editor boots up with the above fix, but I'm seeing some new errors in the console: Uncaught (in promise) Error: Actions must be plain objects. Use custom middleware for async actions.. Not sure if they affect the functionality yet, and if so in what way.

@vindl vindl 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 Oct 11, 2018
@vindl vindl changed the title [WIP] Gutenberg: update packages to latest versions Gutenberg: update packages to latest versions Oct 11, 2018
@vindl vindl force-pushed the update/gutenberg-packages-2 branch 2 times, most recently from c970b90 to a65b888 Compare October 11, 2018 21:46
@gwwar gwwar requested a review from a team October 11, 2018 23:46
@gwwar
Copy link
Contributor

gwwar commented Oct 11, 2018

Error: Actions must be plain objects. Use custom middleware for async actions.

I think that error is from redux-thunk, I tagged Team Calypso in case they had any hints on how to better debug that.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 12, 2018

Error: Actions must be plain objects. Use custom middleware for async actions.

This is an error thrown by Redux itself, when it's told to dispatch an action that's not a plain object ({ type, ... }). In this case, we dispatch a generator function. There should be some middleware that intercepts special actions like this, but there isn't. The @wordpress/data Redux stores only have a promise-middleware that intercept actions that are promises.

I see that the middlewares to handle asyncGenerator and control actions are now available as plugins. Maybe we need to register these plugins when initializing the data stores now? Calling @youknowriad to shed some light on this.

These generator actions come from @wordpress/core-data ands its data store.

My local Calypso uses @wordpress/data@2.1.1 and @wordpress/core-data@2.0.2. Are these versions compatible or do we have a version mismatch?

@vindl vindl force-pushed the update/gutenberg-packages-2 branch from a65b888 to 0becfea Compare October 12, 2018 10:59
@vindl
Copy link
Member Author

vindl commented Oct 12, 2018

Thanks Jarda!

Maybe we need to register these plugins when initializing the data stores now?

FWIW so far I tried registering them when editor mounts but the issue remains.

import { plugins, use } from '@wordpress/data';

...

use( plugins.asyncGenerator );
use( plugins.controls );

@youknowriad
Copy link
Contributor

youknowriad commented Oct 12, 2018

The async generator is deprecated. We'll consolidate all of our actions and resolvers to use the controls plugin.

FWIW so far I tried registering them when editor mounts but the issue remains.

What issue?

@vindl vindl force-pushed the update/gutenberg-packages-2 branch from 0becfea to 9f245e3 Compare October 12, 2018 11:21
@vindl
Copy link
Member Author

vindl commented Oct 12, 2018

@youknowriad the one described in #27778 (comment):

Uncaught (in promise) Error: Actions must be plain objects. Use custom middleware for async actions.

@youknowriad
Copy link
Contributor

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

@vindl
Copy link
Member Author

vindl commented Oct 12, 2018

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. :/


// jquery is only needed in the build for the desktop app
// see electron bug: https://github.com/atom/electron/issues/254
if ( calypsoEnv !== 'desktop' && ! config.isEnabled( 'gutenberg' ) ) {
Copy link
Contributor

@blowery blowery Oct 12, 2018

Choose a reason for hiding this comment

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

We shouldn't use config feature flags this way at build time. Feature flags are meant for runtime decisions, not build time ones.

The problem will become apparent if / when gutenberg is enabled on wpcalypso or staging, but disabled in production. We share a build across those environments, so wpcalypso and staging will likely still have the external set.

@vindl vindl force-pushed the update/gutenberg-packages-2 branch from 6fc60d8 to fdbc969 Compare October 15, 2018 22:19

if ( ! siteId ) {
return;
}

unsubscribe();

if ( ! userId ) {
Copy link
Member

Choose a reason for hiding this comment

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

This check should be never needed. Calypso boot guarantees that if a user is logged in, current user ID is set to the right value from the very moment when the Redux store is created. No need to ever wait for it.

If no one is logged in, the Gutenberg section won't be loaded at all, because it doesn't have the enableLoggedOut flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 06bcef3.

return;
}

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

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

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

I'm not knowledgeable enough for the code part, but after smoke testing Gutenberg and Calypso, I haven't found anything that breaks up all the things.
So, test-wise, this LGTM!

@vindl vindl removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 16, 2018
@vindl vindl merged commit 7dbf8ee into master Oct 16, 2018
@vindl vindl deleted the update/gutenberg-packages-2 branch October 16, 2018 16:52
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants