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

Try implementing a built-in Gutenberg playground #14497

Merged
merged 9 commits into from
Mar 25, 2019
Merged

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Mar 18, 2019

This PR adds a local built-in Gutenberg playground:

  • This playground allows us to test Gutenberg in a WordPress agnostic context
  • It surfaces a lot of shortcomings we have in several styles/components
  • This playground could grow over time to hold more than just a standalone version of the editor. (Thinking about components to test them in isolation...)

Testing instructions

@@ -103,6 +103,7 @@
"mkdirp": "0.5.1",
"node-sass": "4.11.0",
"node-watch": "0.6.0",
"parcel-bundler": "1.12.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using wp-scripts but it's not usable at the moment for this kind of use-cases as it automatically uses wp.* globals. In this particular case I want to bundle the dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add --webpack-no-externals flag to wp-scripts build?

Copy link
Member

Choose a reason for hiding this comment

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

We would have to add support for CSS as well. Does Parcel cover RTL support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parcel can support RTL through post-css plugins. That said, I'd prefer using wp-scripts but there are still things to figure out before:

  • Do we want an --webpack-no-externals. Is it something useful outside of this playground?
  • How do we build CSS with wp-scripts

Copy link
Member

Choose a reason for hiding this comment

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

That's a great question for Core JS chat today :)

We should also check with @front team since they have this create-clould-block tool which scaffolds cloud blocks:
https://github.com/front/create-cloud-block/blob/master/examples/1-simple-block/webpack.config.js

It uses different externals and bundles CSS as well.

*/
import React from 'react';

window.React = React;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not certain why this is needed yet. Maybe a parcel thing.

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't it an issue only because Babel config couldn't be loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So to solve this, I had to explicitely add the pragma config to the .babelrc. I'm not sure why this is needed though as in theory it's already done by the wordpress default preset. 🤔

/**
* WordPress dependencies
*/
import '@wordpress/editor'; // This shouldn't be necessary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still using core/editor in some places in the block editor, it should be refactored properly to avoid the need to import this.

import '@wordpress/block-editor/build-style/style.css';
import '@wordpress/block-library/build-style/style.css';
import '@wordpress/block-library/build-style/editor.css';
import '@wordpress/block-library/build-style/theme.css';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if should have a simpler way to import all the required CSS styles for an npm package.

@youknowriad
Copy link
Contributor Author

I noticed that Gutenberg requires some global styles we're including in edit-post. There are probably some CSS tweaks we can make to make sure these are not necessary to have sane defaults.

A lot of core blocks rely on the REST API... to work properly, if we can make them more generic, that would be a good win.

I like how this playground (and the fact that we can use it as part of our dev workflow) would allow us to detect issues related to the npm packages more easily.

@draganescu
Copy link
Contributor

What is this playground for and how to use it? :)

@youknowriad
Copy link
Contributor Author

This playground is to use/try Gutenberg in a context outside of WordPress. this PR is still very early but you can try it by running npm run playground:start and access http://localhost:1234

@youknowriad youknowriad self-assigned this Mar 19, 2019
@youknowriad youknowriad marked this pull request as ready for review March 19, 2019 08:47
@youknowriad youknowriad requested a review from a team March 19, 2019 08:47

// TODO: Extract these WP Global styles to a mixin
// We can't apply these globally to html or body at the moment because of the WP sidebar/menu
:root {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content of this selector is a "reset" that is applied in Gutenberg but is not applied globally to WordPress as we don't want to mess with the sidebar/topbar. This could be extracted to a mixin as it's useful in edit-widgets as well.

}

a,
div {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a WP-admin style assumed to exist in Gutenberg.

@@ -19,7 +19,7 @@ const FormatToolbar = ( { controls } ) => {
<Slot name={ `RichText.ToolbarControls.${ format }` } key={ format } />
) }
<Slot name="RichText.ToolbarControls">
{ ( fills ) => fills.length &&
{ ( fills ) => fills.length !== 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This renders 0 if there's no fills.

Copy link
Member

Choose a reason for hiding this comment

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

Now, I remember why I was using isEmpty in other places. I reviewed this code myself but I couldn't find a reason why it would be wrong 😅

/cc @ellatrix

Copy link
Member

Choose a reason for hiding this comment

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

This renders 0 if there's no fills.

See also #14579

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR is cool. I think this should be part of the repository as it helps to catch some bad practices like all those hacks with CSS imports :)

.babelrc Outdated Show resolved Hide resolved
@@ -103,6 +103,7 @@
"mkdirp": "0.5.1",
"node-sass": "4.11.0",
"node-watch": "0.6.0",
"parcel-bundler": "1.12.2",
Copy link
Member

Choose a reason for hiding this comment

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

Should we add --webpack-no-externals flag to wp-scripts build?

@youknowriad youknowriad force-pushed the try/playground branch 2 times, most recently from 561c8aa to 3549c93 Compare March 21, 2019 09:42
@youknowriad
Copy link
Contributor Author

Here's the Gutenberg Playground at its current state.

Capture d’écran 2019-03-21 à 4 52 36 PM

I'd argue that sometimes it's easier to work and develop in this separate context, for this reason, I think we should ship it and iterate on it. What do you all think?

@@ -190,7 +191,9 @@
"test-unit:update": "npm run test-unit -- --updateSnapshot",
"test-unit:watch": "npm run test-unit -- --watch",
"test-unit-php": "docker-compose run --rm wordpress_phpunit phpunit",
"test-unit-php-multisite": "docker-compose run -e WP_MULTISITE=1 --rm wordpress_phpunit phpunit"
"test-unit-php-multisite": "docker-compose run -e WP_MULTISITE=1 --rm wordpress_phpunit phpunit",
"playground:build": "npm run build:packages && parcel build playground/src/index.html -d playground/dist",
Copy link
Member

Choose a reason for hiding this comment

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

I have just figured out that we can also use wp-scripts build --config playground/webpack.config.js :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said a lot of time now, I don't see the value of this, it's the same as webpack --config playground/webpack.config.js

Copy link
Member

Choose a reason for hiding this comment

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

Parcel does one more thing I didn't think about initially. It runs the server which is quite important in this particular case. Saying that I convinced myself that it is the best way to proceed as is 😅

.babelrc Outdated
@@ -0,0 +1,8 @@
{
"presets": ["@wordpress/babel-preset-default"],
"plugins": [
Copy link
Member

Choose a reason for hiding this comment

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

I figured out why we need it in the first place:

https://github.com/parcel-bundler/parcel/blob/060db2e2c56f08e223e9a9075035f0998e249763/packages/core/parcel-bundler/src/transforms/babel/jsx.js#L40-L49

Parcel tries to be smart and overrides our configuration.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

There is one blocker which I discovered, there is some JS errors which breaks some behaviors like removing blocks:

error-js

.babelrc Outdated
@@ -0,0 +1,8 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this config inside playground folder and maybe remove babel.config.js which is something wp-scripts build/start will use in the same form regardless?

Copy link
Member

@gziolo gziolo Mar 25, 2019

Choose a reason for hiding this comment

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

There is no plan of supporting babel.config.js with Parcel:
parcel-bundler/parcel#2110 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved :)

@youknowriad
Copy link
Contributor Author

@gziolo Actually, there are a lot of these issues and I don't consider them a blocker personally. These are related to the fact that we require plugins for the registry... some API fetch networks should be done only if the WP context... I consider these as separate issues that if solved will improve the reusability of the block editor module and the block library.

Related #14043

@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Type] Task Issues or PRs that have been broken down into an individual action to take labels Mar 25, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Should we add a new label Playground?

Let's move forward and address those issues separately then. My biggest concerns was the fact that we would have 2 Babel configs in the root folders which would make it confusing to maintain them. It was addressed so let's 🚢

@youknowriad youknowriad merged commit 8625b82 into master Mar 25, 2019
@youknowriad
Copy link
Contributor Author

Thanks for the review @gziolo

@youknowriad youknowriad deleted the try/playground branch March 25, 2019 12:39
@aduth
Copy link
Member

aduth commented Mar 25, 2019

Should this be highlighted in tomorrow's Core JS meeting?

Having a hard time surmising from the conversation: Is Parcel intended to become a permanent tool? My main thought would be on the impact on install time (I think I measured it around ~8000 new dependencies) and maintaining multiple build toolchains, though I guess the proposed benefit in the latter case is that there's not so much configuration around Parcel to be maintaining.

@youknowriad
Copy link
Contributor Author

Yep, definitely worth highlighting.

Parcel itself is an implementation detail as if I wanted to do the same with webpack, it would have required a lot of config. The dependency overhead is a real issue though, If I were to choose I'd actually think we should see if our current usage of webpack could be replaced by parcel :) but I don't mind the opposite.

@gziolo
Copy link
Member

gziolo commented Mar 26, 2019

Having a hard time surmising from the conversation: Is Parcel intended to become a permanent tool?

Yes, for the playground. It was way much simpler to proceed this way as @wordpress/scripts is optimized towards a WordPress-based environment:

  • WordPress globals and other libraries handled as externals
  • no default server support for standalone usage
  • no built-in integration with HTML files

The downside is that Parcel comes with its own set of development dependencies and does some magic with Babel config which caused some issues as we had to override the default config we use.

@@ -12,3 +12,6 @@ phpcs.xml
yarn.lock
docker-compose.override.yml
/wordpress

playground/dist
.cache
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be in .eslintignore ? Ignored tests (#14997) ? I worry its presence could add some significant folder-scan overhead for some common tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a config for .eslintignore to automatically include .gitignore?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a config for .eslintignore to automatically include .gitignore?

Not that I'm aware of, though I do sympathize with the goal to try to consolidate how we consider these "ignored" files.

Related: eslint/eslint#5848

@comerc
Copy link

comerc commented Oct 1, 2019

/home/aka/gutenberg/playground/src/index.js:28:7: Cannot resolve dependency '@wordpress/components/build-style/style.css'

First time need command: npm run playground:build

@landsman
Copy link

This was killed? I do not see playground folder in master anymore.

@aduth
Copy link
Member

aduth commented Feb 13, 2020

@landsman As far as I know, it was integrated into the "Storybook" (storybook folder), where the editor is just one of many things that can be viewed in a standalone, static context.

You can view it (the block editor) online as well:

https://wordpress.github.io/gutenberg/?path=/story/playground-block-editor--default

(Not sure how frequently that is kept in sync with current master, otherwise you can run it from npm run storybook:dev)

@gziolo
Copy link
Member

gziolo commented Feb 14, 2020

https://wordpress.github.io/gutenberg/?path=/story/playground-block-editor--default

(Not sure how frequently that is kept in sync with current master, otherwise you can run it from npm run storybook:dev)

When Travis works properly, then this Storybook instance should be updated to use the latest changes on every merge to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants