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

Handle CSS build process with Webpack #6057

Merged
merged 8 commits into from
Apr 13, 2021
Merged

Conversation

delawski
Copy link
Collaborator

@delawski delawski commented Apr 8, 2021

Summary

So far, all stylesheets that were not imported in JS files were handled with plain CLI commands. The simple "build" process consisted of copying all CSS files from ./assets/css/src to ./assets/css and generating RTL stylesheets with the rtlcss CLI command.

With this update, all files in the ./assets/css/src folder are handled by the new 'Styles' Webpack entry point. Thanks to that, they are watched and rebuilt on the fly with the regular npm run dev task. Also, it is now possible to use the SASS/SCSS preprocessing, if needed.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

So far, all stylesheets that were not imported in JS files were handled with plain CLI commands. The simple "build" process consisted of copying all CSS files from `./assets/css/src` to `./assets/css` and generating RTL stylesheets with the `rtlcss` CLI command.

With this update, all files in the `./assets/css/src` folder are handled by the new 'Styles' Webpack entry point. Thanks to that, they are watched and rebuilt on-the-fly with the regular `npm run dev` task. Also, it is now possible to use the SASS/SCSS preprocessing, if needed.
@delawski delawski added Enhancement New feature or improvement of an existing one CSS WS:UX Work stream for UX/Front-end labels Apr 8, 2021
@delawski delawski added this to the v2.1 milestone Apr 8, 2021
@delawski delawski self-assigned this Apr 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2021

Plugin builds for 075b1ae are ready 🛎️!

@delawski
Copy link
Collaborator Author

delawski commented Apr 8, 2021

Since the CI complained about missing build:css npm task, I've added it back. I think it could possibly be removed from the CI since it doesn't make sense to run the same underlying task (wp-scripts build) twice, for build:js and for build:css.

@swissspidy
Copy link
Collaborator

Doesn't this cause tons of empty JS chunks for these CSS files?

@delawski
Copy link
Collaborator Author

delawski commented Apr 8, 2021

Doesn't this cause tons of empty JS chunks for these CSS files?

@swissspidy Well, yes, it does. Obviously, not the nicest side effect. Do you think there's a way to get rid of them?

@delawski
Copy link
Collaborator Author

delawski commented Apr 9, 2021

It seems that it's possible to not generate those empty chunks by using the ignore-emit-webpack-plugin. I've borrowed this idea from @wordpress/scripts and applied it in e64576a.

This, along with the previous commit that disables the dependency-extraction-webpack-plugin for stylesheets, ensures that only CSS files are being built by Webpack in the styles task.

package.json Show resolved Hide resolved
@westonruter
Copy link
Member

Since the CI complained about missing build:css npm task, I've added it back. I think it could possibly be removed from the CI since it doesn't make sense to run the same underlying task (wp-scripts build) twice, for build:js and for build:css.

Makes sense. I've removed all traces of build:css in de08737.

@westonruter
Copy link
Member

Comparing develop with this PR, I see the output here is even better as it now minifies the CSS whereas before it was not:

image

@westonruter
Copy link
Member

And when comparing with the development build, I can see now we get vendor-prefixed properties and sourcemaps whereas before we didn't:

image

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

The output looks great.

The webpack config changes I'm not very familiar with, so I request @pierlon to verify.

Otherwise, looks great to merge!

@@ -340,6 +341,80 @@ const settingsPage = {
],
};

const styles = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that Gutenberg had a similar problem and so created the FixStyleWebpackPlugin plugin to prevent those JS chunks from being generated. That plugin would only apply chunk assets that match the style cache group, however (that is, it would only apply to files named style.css).

Although the plugin is included in sharedConfig.plugins, it would not be applying anything as we're not including the aforementioned style optimization cache group.

Comment on lines +384 to +386
...sharedConfig.plugins.filter(
( plugin ) => plugin.constructor.name !== 'DependencyExtractionWebpackPlugin',
),
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary if the IgnoreEmitPlugin plugin was before the DependencyExtractionWebpackPlugin plugin, but this works 👍.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to avoid adding that but haven't thought about the order of the plugins and so the .asset.php files were always generated. I guess we could improve the implementation in the future.

Comment on lines 404 to 408
const match = entryPath.match( /^.*\/(.*)\..*$/ );

return match && match[ 1 ] ? `${ match[ 1 ] }.js` : null;
} )
.filter( Boolean );
Copy link
Contributor

@pierlon pierlon Apr 13, 2021

Choose a reason for hiding this comment

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

Does the match regex need to be this generic? Since all the entry points are CSS files this could be made simpler:

Suggested change
const match = entryPath.match( /^.*\/(.*)\..*$/ );
return match && match[ 1 ] ? `${ match[ 1 ] }.js` : null;
} )
.filter( Boolean );
const match = entryPath.match( /([^\/]+)\.s?css$/ );
return `${ match[ 1 ] }.js`;
} );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The entry points may also be SCSS files, but I guess the regex could still get simplified to something like:

/([^\/]+)\.s?css$/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, that works 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.

@pierlon Would you apply that change and verify it produces the expected output? Then we can merge this PR if I'm not mistaken, at least for beta2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that an empty JS chunk is also being generated for the wp-components entrypoint on the "Settings page" webpack config. We can either move it to the "Styles" webpack config, or extract these set of changes into a new webpack plugin so that we can apply it to multiple webpack configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go for the former right now, and then deal with the latter later on. I can create an issue for that if needed.

@delawski
Copy link
Collaborator Author

QA passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Enhancement New feature or improvement of an existing one WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants