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

Scripts: Add CSS support to start and build scripts #21730

Merged
merged 12 commits into from
May 21, 2020
Merged

Scripts: Add CSS support to start and build scripts #21730

merged 12 commits into from
May 21, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 20, 2020

Description

Closes #14801.

This PR adds new capability to build and start scripts that automates handling CSS, SASS or SCSS files by importing them from JavaScript code.

One of the coolest webpack features is that you can also include any other type of file, besides JavaScript, for which there is a loader. This means that the same benefits listed above for JavaScript (e.g. explicit dependencies) can be applied to everything used in building a website or web app.

More details about assets management in webpack at https://webpack.js.org/guides/asset-management/.

It builds upon the work @fabiankaegy started in #14847 where he added loaders for CSS, SCSS/SASS that compile and afterward run PostCSS.

It is heavily influenced by the solution shared by @phpbits in https://jeffreycarandang.com/create-gutenberg-block-plugin-wp-scripts-postcss-build/.

Implementation details

I made the following assumptions:

  1. When you are writing JavaScript code for the block it's going to be rendered in the editor so it's going to be used in WP-Admin only. That's why all imported CSS files are dumped into one chunk named after the entry point which is index.js and thus it becomes index.css by default. Now, when you use import style.css; (or with different file extensions) it gets bundled into style.css file that is meant to be used both on the front-end and in the editor.
  2. There is a second reason why I opted for style to be tackled differently. You can have multiple entry points as described in the docs:
    wp-scripts start entry-one.js entry-two.js --output-path=custom
    In fact, you can also have only one entry point and name it differently if you will. The chances that you name an entry point style is close to zero. If we would flip the approach and make editor.(sc|sa|c)ss a special case, the chances that someone would name their chunk editor would be much higher.

New npm package

I had to extract a new WordPress npm package @wordpress/postcss-config that allows sharing PostCss configuration between @wordpress/scripts, Storybook configuration and Gutenberg script that builds local packages.

How has this been tested?

This is how I tested it inside Gutenberg:

  1. I executed npx wp-create-block my-test-block to generate some random block with @wordpress/scripts setup.
  2. I added a new file my-test-block/src/editor.scss with some styles for the editor.
  3. I added a new file my-test-block/src/style.scss with some styles for both the front-end and the editor.
  4. I updated my-test-block/src/index.js to import both files created:
    import './editor.scss';
    import './style.scss';
  5. Now everything is almost ready for testing but we need to ensure that we run the latest @wordpress/scripts rather than the one bundled in the scaffolded plugin.
  • I removed webpack.config.js file to let wp-scripts use the default config provided.
  1. I executed:
    npx wp-scripts build my-test-block/src/index.js --output-path "my-test-block/build"
  2. I could see two CSS files generated in my-test-block/build folder:
  • index.css – containing content from src/editor.scss
  • style.css – containing content from src/style.scss

Testing multiple entry points

Bonus points are for testing multiple entry points with the following command:

npx wp-scripts build my-test-block/src/index.js my-test-block/src/other-file.js --output-path "my-test-block/build"

It requires adding other-file.js and some additional import statements pointing to CSS files. It should produce 3 CSS files:

  • index.css – containing content from src/editor.scss listed in index.js
  • other-file.css – containing content referenced in the additional other-file.js file
  • style.css – containing content from src/style.scss or other files that end with style.(sc|sa|c)ss

Testing @wordpress/postcss-config

I also ensured that Gutenberg and Storybook still work:

  • npm run dev
  • npm run storybook:dev

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement. labels Apr 20, 2020
@gziolo gziolo self-assigned this Apr 20, 2020
@gziolo gziolo marked this pull request as draft April 20, 2020 14:48
@github-actions
Copy link

github-actions bot commented Apr 20, 2020

Size Change: +287 kB (25%) 🚨

Total Size: 1.12 MB

Filename Size Change
build/annotations/index.js 3.62 kB +1 B
build/block-editor/index.js 105 kB +568 B (0%)
build/block-editor/style-rtl.css 10.9 kB +45 B (0%)
build/block-editor/style.css 10.9 kB +43 B (0%)
build/block-library/editor-rtl.css 7.17 kB -19 B (0%)
build/block-library/editor.css 7.17 kB -19 B (0%)
build/block-library/index.js 119 kB +686 B (0%)
build/block-library/style-rtl.css 7.48 kB +1 B
build/block-library/style.css 7.48 kB -1 B
build/block-library/theme-rtl.css 684 B +1 B
build/block-library/theme.css 686 B +1 B
build/block-serialization-default-parser/index.js 1.88 kB -1 B
build/blocks/index.js 48.1 kB -3 B (0%)
build/components/index.js 190 kB +7.78 kB (4%)
build/components/style-rtl.css 17.1 kB +35 B (0%)
build/components/style.css 17.1 kB +34 B (0%)
build/compose/index.js 9.24 kB +2.57 kB (27%) 🚨
build/core-data/index.js 11.4 kB +25 B (0%)
build/data-controls/index.js 1.29 kB -2 B (0%)
build/data/index.js 8.42 kB -3 B (0%)
build/date/index.js 5.47 kB +2 B (0%)
build/deprecated/index.js 771 B -1 B
build/dom-ready/index.js 568 B -1 B
build/edit-navigation/index.js 6.63 kB +28 B (0%)
build/edit-navigation/style-rtl.css 857 B +25 B (2%)
build/edit-navigation/style.css 856 B +25 B (2%)
build/edit-post/index.js 302 kB +274 kB (90%) 🆘
build/edit-post/style-rtl.css 12.2 kB +11 B (0%)
build/edit-post/style.css 12.2 kB +11 B (0%)
build/edit-site/index.js 12.9 kB +891 B (6%) 🔍
build/edit-site/style-rtl.css 5.31 kB +91 B (1%)
build/edit-site/style.css 5.31 kB +91 B (1%)
build/edit-widgets/index.js 7.73 kB -137 B (1%)
build/edit-widgets/style-rtl.css 4.59 kB -102 B (2%)
build/edit-widgets/style.css 4.59 kB -102 B (2%)
build/editor/index.js 44.6 kB +306 B (0%)
build/editor/style-rtl.css 5.06 kB -14 B (0%)
build/editor/style.css 5.06 kB -15 B (0%)
build/element/index.js 4.65 kB -1 B
build/format-library/index.js 7.64 kB +1 B
build/hooks/index.js 2.13 kB +1 B
build/i18n/index.js 3.56 kB +1 B
build/keyboard-shortcuts/index.js 2.51 kB -1 B
build/keycodes/index.js 1.94 kB +2 B (0%)
build/list-reusable-blocks/index.js 3.13 kB -3 B (0%)
build/media-utils/index.js 5.29 kB +3 B (0%)
build/notices/index.js 1.79 kB +1 B
build/nux/index.js 3.4 kB -1 B
build/plugins/index.js 2.56 kB -2 B (0%)
build/primitives/index.js 1.5 kB +1 B
build/rich-text/index.js 14.8 kB +3 B (0%)
build/url/index.js 4.02 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/dom/index.js 3.11 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@gziolo gziolo requested review from oandregal and ItsJonQ April 20, 2020 15:12
@gziolo
Copy link
Member Author

gziolo commented Apr 20, 2020

As for Sass, it's slowly falling to the wayside in favor of pure CSS (currently via PostCSS). People will keep using it the same way people keep using jQuery, but the percentage of users will slowly diminish unless authority figures or projects actively encourage its use. I suggest providing Sass compilation as an option at the top of the stack as proposed, so that developers can use custom properties and all the other new CSS goodness alongside Sass and then process everything through PostCSS.

@mor10 – just to make sure, do we cover the use cases with the current proposal? I'm not much into the specifics of CSS support in webpack but I'm motivated to land the initial implementation :)

@gziolo
Copy link
Member Author

gziolo commented Apr 20, 2020

To contextualize the above comment: anecdotally, every time I've spoken with theme or plugin developers at a WordCamp over the past two months they've asked when SASS support is coming. As I've not been involved in the project I haven't had visibility into the answer, but it's a very common question. And most of those developers are following Webpack patterns and guidelines from the broader web community, which tend to favor importing the styles within the module. This is something I think therefore that it is very important we support, because if we say "we use all the same tools you do, but you have to use them in this strange (to your perspective) way" that's quite off-putting for any potential future contributors or other web developers who want to use patterns they know.

@kadamwhite – I echo all that. I heard similar questions so many times. I also had to help people to fix issues caused by the their own try to add it to the default config that I'm in the position that it's more beneficial in terms of maintenance to add CSS support to wp-scripts natively than answer the same questions all over again on GitHub, Slack and at WordCamps.

@gziolo
Copy link
Member Author

gziolo commented Apr 28, 2020

I think I figured out how to add CSS import support to wp-scripts build/start, there might be still some configurations missing but I think it’s solid enough to collect feedback. I still need to add all the details how it's designed to work and how it can be tested. I plan to take care of it tomorrow morning.

@gziolo gziolo marked this pull request as ready for review April 28, 2020 17:25
@gziolo gziolo requested a review from chrisvanpatten as a code owner April 28, 2020 17:25
@@ -29,9 +29,7 @@
],
"main": "index.js",
"dependencies": {
"autoprefixer": "^9.4.5",
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out all 3 packages aren't used in the package ...

splitChunks: {
cacheGroups: {
styles: {
name: '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.

The interesting part here is that all other styles that don't match the regular expression included in test get combined into another chunk that is named after an entry point:
index.js -> index.css

It even works with multiple entry points:
one.js -> one.css
two.js -> two.css

All files that end with style.(sc|sa|c) are combined into one file -> style.css

Copy link
Member

Choose a reason for hiding this comment

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

@gziolo When I try to import css files with a name different from style.(sc|sa|c)ss everything just gets dumped into one chunk called index.css. Which is not what you described here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@fabiankaegy what you described is exactly what I was trying to explain in my comment. It looks like I did it poorly 🙃

I made the following assumption – you are writing JavaScript code for the block editor so it's going to be used in WP-Admin only. That's why all imported CSS files are dumped into one chunk named after the entry point which is index.js and thus index.css by default. Now, when you use import style.css; (or with different file extensions) it gets bundled into style.css file that is meant to be used both on the front-end and in the editor. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Okay so with that the workflow would be enqueueing the index.css file in the editor and the style.css in the both places?

Copy link
Member

Choose a reason for hiding this comment

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

If so that’s fine but since in one case the naming of the imported file is used and in the other place it isn’t I think this needs to be made clear in the documentation.

My initial gut feeling of how it should work was that it automatically combines all files with the same name into a chunk with that name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's all correct. Documentation is quite important here :)

It reminds also about the second reason why I opted for style to be tackled differently. You can have multiple entry points as described in the docs:

wp-scripts start entry-one.js entry-two.js --output-path=custom

In fact, you can also have still one entry point and name it differently if you will. The chances that you name an entry point style is close to zero. If we would flip the approach and make editor.(sc|sa|c)ss special case, the chances that someone would name their chunk editor would be much higher.

@fabiankaegy
Copy link
Member

Currently the @wordpress/postcss-config gets imported right into the webpack.config.js file. How can we make it as easy as possible to extend or override this config? It would be awesome if we could allow for just adding a postcss.config.js file to be root of the project and the script then referencing that one instead of the one from the @wordpress/postcss-config package.

@gziolo
Copy link
Member Author

gziolo commented Apr 29, 2020

Currently the @wordpress/postcss-config gets imported right into the webpack.config.js file. How can we make it as easy as possible to extend or override this config? It would be awesome if we could allow for just adding a postcss.config.js file to be root of the project and the script then referencing that one instead of the one from the @wordpress/postcss-config package.

I wasn't aware that postcss.config.js is a thing :) It looks like it's standardized: https://github.com/michael-ciniawsky/postcss-load-config. I'm more than happy to add support for it in a follow-up PR. It will be very similar to how we handle all other configs (Babel, ESLint, etc.) 👍

@gziolo
Copy link
Member Author

gziolo commented Apr 29, 2020

I updated the description and included all the details on how I tested this PR. The only missing part is documentation in the advanced section of @wordpress/scripts README file explaining how to take advantage of CSS/SASS/SCSS import support.

packages/postcss-config/package.json Outdated Show resolved Hide resolved
packages/scripts/README.md Show resolved Hide resolved
packages/scripts/config/webpack.config.js Outdated Show resolved Hide resolved
packages/scripts/config/webpack.config.js Outdated Show resolved Hide resolved
packages/postcss-config/lib/index.js Outdated Show resolved Hide resolved
packages/postcss-config/lib/index.js Outdated Show resolved Hide resolved
"minimist": "^1.2.0",
"node-sass": "^4.13.1",
Copy link
Member

Choose a reason for hiding this comment

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

I hope we don't come to regret foisting this troublesome dependency upon consumers of @wordpress/scripts 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

It is what it is, we use it in Gutenberg as well.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe separately, it's something to consider to the general problem of the amount (and difficulty) of the specific dependencies we ship here. Whether it's something we can defer with some sort of lazy importing, or splitting off to more granular packages, or some sort of peer dependency pattern to leverage the features the developer opts in to.

I just worry that @wordpress/scripts tries to do everything and the pattern to ship it all in the same package this way won't doesn't scale well to accommodate.

Copy link
Member

Choose a reason for hiding this comment

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

Though, to be clear, my issue with SASS dependency specifically is less of size, and more of how it's built from source, and all the usability problems that stem from this (e.g. Node version correspondence).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest changing this to Dart SASS (sass package). It's the primary implementation for SASS, and saves contributors all the native compilation trouble node-sass brings. sass-loader picks it up without any additional configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yscik and @aduth – I filed #22729 to continue the discussion. I don't mind switching. In this PR, the goal was to mirror as much as possible from what Gutenberg uses – for better or worse :)


When you run the build using the default command `wp-scripts build` (also applies to `start`) in addition to the JavaScript file `index.js` generated in the `build` folder, you should see two more files:
1. `index.css` – all imported CSS files are bundled into one chunk named after the entry point, which defaults to `index.js`, and thus the file created becomes `index.css`. This is for styles used only in the editor.
2. `style.css` – imported `style.css` file(s) (applies to SASS and SCSS extensions) get bundled into one `style.css` file that is meant to be used both on the front-end and in the editor.
Copy link
Member

Choose a reason for hiding this comment

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

Have we previously made such assumptions about how @wordpress/scripts is used for bundling for the editor, vs. being purely general-purpose in its goals? I need to spend some time to look at some of the history here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's something I proposed based on the previous implementation from #14847, an article from @phpbits https://jeffreycarandang.com/create-gutenberg-block-plugin-wp-scripts-postcss-build/, create-guten-block (https://github.com/ahmadawais/create-guten-block/blob/bdc40192fc7fa30519d821e2aa835b2aa6ef7534/packages/cgb-scripts/config/webpack.config.prod.js#L109-L118) or the old implementation in Gutenberg before we moved to packages. I made an assumption that it became a standard approach in the community.

@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

I plan to merge this PR at the end of the week to ensure it's included in the npm release that should happen next week.

packages/scripts/config/webpack.config.js Outdated Show resolved Hide resolved
@gziolo gziolo merged commit 574a77c into master May 21, 2020
@gziolo gziolo deleted the pr/14847 branch May 21, 2020 18:57
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 21, 2020
@gziolo
Copy link
Member Author

gziolo commented May 21, 2020

Thank you all for making this happen. Let’s iterate based on feedback from the people using it in their projects 🎉

@gziolo
Copy link
Member Author

gziolo commented May 29, 2020

It's now published to npm as part of v10.0.0!

oekeur added a commit to greenpeace/planet4-gpnl-plugin-gutenberg-blocks that referenced this pull request Jan 11, 2021
NOTE:
[this @wordpress/scripts pr](WordPress/gutenberg#21730) breaks mini-css-extract functionality. Upgrading @wordpress/scripts above 9.0.0 WILL break our webpack config
oekeur added a commit to greenpeace/planet4-gpnl-plugin-gutenberg-blocks that referenced this pull request Jan 11, 2021
NOTE:
[this @wordpress/scripts pr](WordPress/gutenberg#21730) breaks mini-css-extract functionality. Upgrading @wordpress/scripts above 9.0.0 WILL break our webpack config
oekeur added a commit to greenpeace/planet4-gpnl-plugin-gutenberg-blocks that referenced this pull request Jan 20, 2021
* chore: dependency updates and optimization

NOTE:
[this @wordpress/scripts pr](WordPress/gutenberg#21730) breaks mini-css-extract functionality. Upgrading @wordpress/scripts above 9.0.0 WILL break our webpack config

* fix: update child theme handle
oekeur added a commit to greenpeace/planet4-gpnl-plugin-gutenberg-blocks that referenced this pull request Jan 20, 2021
* chore: dependency updates and optimization

NOTE:
[this @wordpress/scripts pr](WordPress/gutenberg#21730) breaks mini-css-extract functionality. Upgrading @wordpress/scripts above 9.0.0 WILL break our webpack config

* fix: update child theme handle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding SASS / PostCSS build process to wp-scripts package
6 participants