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 npm packages to latest versions #28000

Merged
merged 2 commits into from
Oct 29, 2018

Conversation

vindl
Copy link
Member

@vindl vindl commented Oct 22, 2018

Changes proposed in this Pull Request

Updates Gutenberg npm packages to latest versions.

It seems like this update will not require as much fixes as the ones before it. :)

Testing instructions

  1. Navigate to http://calypso.localhost:3000/gutenberg/
  2. Verify that there are no new errors in console.
  3. Try inserting some content and verify that publishing still works.
  4. Smoke test Calypso and check bundle sizes.

On hold until new version of components package, that contains WordPress/gutenberg#10943 fix, is published.

@vindl vindl added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Gutenberg Working towards full integration with Gutenberg labels Oct 22, 2018
@vindl vindl self-assigned this Oct 22, 2018
@matticbot
Copy link
Contributor

@vindl vindl requested a review from a team October 22, 2018 23:04
@@ -1,4 +1,4 @@
@import '../../../node_modules/@wordpress/components/build-style/style';
// @import '../../../node_modules/@wordpress/components/build-style/style';
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @mmtr since you've worked on this. If I leave the style include here I'm running into:

{
  "status": 1,
  "file": ".../wp-calypso/node_modules/@wordpress/components/build-style/style.css",
  "line": 1615,
  "column": 27,
  "message": "Function shade is missing argument $percent.",
  "formatted": "Error: Function shade is missing argument $percent.\n        on line 1 of client/devdocs/gutenberg-components/style.scss\n        from line 7 of assets/stylesheets/sections/devdocs.scss\n>>       background: #0085ba shade(15%); }\n\n   --------------------------^\n"
}

The mentioned shade function has the argument percent passed at that line if you check the built file. Any ideas what might be causing this?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that the shade function shouldn't be in the generated CSS file given that it's not a CSS function AFAIK but I can see it on the original SCSS file (added on WordPress/gutenberg#7621). I'm sure that function should be converted to something else during the CSS build process.

I see the shade function is also used in other components (i.e. Button), but in all of them the function is an argument of the color function, so maybe the issue is on Gutenberg and it can be solved if we replace background: theme(primary) shade(15%); with background: color(theme(primary) shade(15%)); on the original SCSS file.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that will solve the issue. Gutenberg uses postcss-color-function which provides the color interface and shade as one of their color adjusters.

I can open a PR fixing that as soon as I got access to the Gutenberg repo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@gziolo could we add @mmtr to the repo to file the above fix? :)

Copy link
Member

Choose a reason for hiding this comment

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

I already asked for that in p1540284879000100-slack-gutenberg. I think only @mtias can add me.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, him and @pento :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmtr if you don't want to wait feel free to start a PR from your fork too, but we'll definitely want to provide you with access for future fixes.

Also, it's curious that the same import wasn't causing issues for editor shell in Calypso:
https://github.com/Automattic/wp-calypso/blob/master/assets/stylesheets/sections/gutenberg-editor.scss#L19
That's why I suspected it had something to do with differences in DevDocs setup at first.

Copy link
Member

Choose a reason for hiding this comment

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

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 Miguel!

@gziolo
Copy link
Member

gziolo commented Oct 23, 2018

It seems like this update will not require as much fixes as the ones before it. :)

We are close to the real API freeze, one week left :D

@Copons
Copy link
Contributor

Copons commented Oct 23, 2018

I see two errors I haven't seen yet:

screenshot 2018-10-23 at 11 32 13

First is in: calypso/vendors~async-load-design~async-load-design-blocks~async-load-design-playground~async-load-gutenberg~81da215c.704af1af3a9082fb9a8d.min.js

Second in: calypso/vendors~build.83bedb58be69695ad3b0.min.js

Other than that, though, I've smoke tested Guten, and it's working as well as before. :)

@vindl
Copy link
Member Author

vindl commented Oct 23, 2018

Thanks for testing @Copons! The first one is already present in master, since the importing of Jetpack blocks (#27864), and it's kind of expected (translations issue).

I haven't noticed the second one before (will have to re-test), but it sounds like the problem with API path mapping. If there is some specific action that triggers it, knowing that would be helpful.

@Copons
Copy link
Contributor

Copons commented Oct 23, 2018

Thanks for testing @Copons! The first one is already present in master, since the importing of Jetpack blocks (#27864), and it's kind of expected (translations issue).

Oh, apparently somebody forgot to pull master, eh! 😤

I haven't noticed the second one before (will have to re-test), but it sounds like the problem with API path mapping. If there is some specific action that triggers it, knowing that would be helpful.

I thought it popped up upon first load, but in fact it is triggered by the sidebar.
If the window is large enough that the sidebar is open by default, the error is there on load; otherwise, it triggers when opening the sidebar.

@blowery
Copy link
Contributor

blowery commented Oct 23, 2018

@vindl renovate will automatically generate PRs for updates to the WordPress monorepo (see #27592). In the future, could we please piggyback on those PRs instead of opening separate ones? It keeps the shrinkwrap normalized, gives us automatic release notes, and folks know where to look for module updates and makes is easier to report on the number of PRs related to managing dependencies. Feel free to continue on here and if you need any help, we're glad to assist.

Thanks much!

@blowery
Copy link
Contributor

blowery commented Oct 23, 2018

On the CSS issue, I noticed that we're pulling in the built CSS from Gutenberg. wc-admin uses a slightly different tact, pulling in the SCSS and processing it with postcss to theme out some colors.

Is that something we'd be interested in doing for Calypso? If it is, is there an issue tracking it? Probably a bit related to #26910

@vindl
Copy link
Member Author

vindl commented Oct 23, 2018

@blowery sure! I wasn't aware of all those benefits that renovate brings. If you want I can close this one too and continue there.

My main motivation so far was to merge these updates as soon as possible, because they might affect all the other areas of our work. Is there a way for me to trigger those renovate PRs manually just for WordPress monorepo?

@vindl
Copy link
Member Author

vindl commented Oct 23, 2018

On hold until new version of components package, that contains WordPress/gutenberg#10943 fix, is published.

@vindl
Copy link
Member Author

vindl commented Oct 23, 2018

Is that something we'd be interested in doing for Calypso? If it is, is there an issue tracking it? Probably a bit related to #26910

I believe we discussed this briefly at one point, and for now we are not planning to allow theming for Calypso integration (cc @gwwar @Copons please correct me if I got this wrong).

@blowery
Copy link
Contributor

blowery commented Oct 23, 2018

@vindl it's fine here! I just wanted to make sure you were aware of the renovate PRs. :)

@vindl
Copy link
Member Author

vindl commented Oct 24, 2018

I thought it popped up upon first load, but in fact it is triggered by the sidebar.
If the window is large enough that the sidebar is open by default, the error is there on load; otherwise, it triggers when opening the sidebar.

@Copons the problem here seems to be that WP_REST_Themes_Controller was only recently merged into Gutenberg (WordPress/gutenberg#10518), and our plugin version running on Dotcom doesn't have that update yet (failing request to https://public-api.wordpress.com/wp/v2/sites/example.wordpress.com/themes?status=active). I think we can ignore this error, since it will resolve itself once our backend code is updated.

Added some additional fixes to make the editor boot.
@vindl
Copy link
Member Author

vindl commented Oct 29, 2018

I updated all of the related packages again to the newest versions and introduced a couple of fixes that are needed for editor to boot (will point those out in inline comments). There is one new error regarding the rest route that probably requires a new mapping in middleware, and I'm planning to tackle that in a separate PR in order not to drag this one further. Otherwise, this is ready for another round of reviews and testing.

@@ -38,7 +38,6 @@ function registerDataPlugins( userId ) {
const storageKey = 'WP_DATA_USER_' + userId;

use( plugins.persistence, { storageKey: storageKey } );
use( plugins.asyncGenerator );
Copy link
Member Author

Choose a reason for hiding this comment

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

This has been removed from the latest version of @wordpress/data package: https://github.com/WordPress/gutenberg/blob/master/packages/data/CHANGELOG.md#300-2018-10-29

@@ -58,9 +57,6 @@ function Header( {
isToggled={ isEditorSidebarOpened }
aria-expanded={ isEditorSidebarOpened }
>
<DotTip id="core/editor.settings">
Copy link
Member Author

Choose a reason for hiding this comment

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

This was causing the editor to break, so I'm removing it since this is the only usage of DotTip in edit post, and we don't want to use the guided tour anyway. I'm suspecting its related to some specific error with @wordpress/nux package and not to our integration.

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.

Smoke tested and couldn't find any differences with the current wpcalypso version, except the uncaught error I've already reported a few days ago, but we can handle that in a follow up.

In other words, 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 29, 2018
@vindl vindl merged commit 4c6ce4b into master Oct 29, 2018
@vindl vindl deleted the update/gutenberg-packages-3 branch October 29, 2018 13: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.

None yet

6 participants