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

Update the default CSS parsing/combining/minifying tools #9263

Merged
merged 16 commits into from Nov 22, 2017

Conversation

Projects
None yet
4 participants
@hwillson
Member

hwillson commented Oct 27, 2017

The minifier-css package is currently using the following outdated (and abandoned) npm packages:

  • css-parse
  • css-stringify

This has led to issues like #7075, where a PR has been provided to the upstream repo to fix the issue, but it hasn't been (and won't be) accepted. As discussed in #7075 (comment), we could consider forking and maintaining our own versions of the above packages, but adding more to Meteor's maintenance work load seems less than ideal.

This PR aims to modernize Meteor's CSS build process (in baby steps), by leveraging today's most popular CSS transformation tool - postcss. In this PR, postcss is used to replace the functionality provided by the above packages, as well as remove the need for a custom internal minification process. This PR does not open up Meteor's CSS build process by allowing 3rd party postcss plugins to be run (like the Meteor package juliancwirko:postcss does). It simply leverages postcss to handle the tasks that Meteor's CSS build process is already handling.

Integrating postcss like this could be considered a first step towards modernizing Meteor's CSS build process. With these changes in place, at some point in the future we could consider opening things up further, to allow 3rd party postcss plugins to be run, as part of the existing CSS build process. This could introduce a lot of new capabilities to Meteor's CSS build system, by leveraging postcss's vast (and growing) plugin ecosystem. Food for future thought anyways.

While the changes outlined in this PR are working (verified in development and production), I've marked it as a work in progress for now. I'd like to do more testing, benchmarking and performance tuning (I'll share those performance numbers here shortly) before considering this ready for review.

One final note regarding backwards compatibility. While the minifier-css package is an internal package, it's definitely possible people are relying on it externally. I've tried to make all changes to its API backwards compatible, but in some cases this isn't really possible (well, it's possible, but not easily). The abstract syntax tree that postcss works with is different than the AST css-parse worked with, so some public functions like CssTools.parseCss will now return an AST in a different format. If this is deemed to be an issue we can consider adding in a mapping layer to covert the new AST to the old AST.

More details shortly - thanks!

hwillson added some commits Oct 27, 2017

Update the default CSS parsing/combining/minifying tools
The `minifier-css` package is currently using outdated
(and abandoned) npm packages (`css-parse` and `css-stringify`),
as part of its parsing/minification process. This commit
replaces those packages with the robust, modern and maintained
`postcss` package.

@hwillson hwillson changed the title from Update the default CSS parsing/combining/minifying tools to [WIP] Update the default CSS parsing/combining/minifying tools Oct 27, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Oct 27, 2017

Just to add - I'm aware of the self-test failures, and will have them resolved shortly. Thanks!

@@ -1,39 +0,0 @@
import selftest, {Sandbox} from '../tool-testing/selftest.js';

This comment has been minimized.

@hwillson

hwillson Oct 28, 2017

Member

Quick note regarding why I've removed this test (and its supporting test app). The need to split stylesheets up with more than 4095 selectors was required to support IE 9. All modern (and supported) browsers can handle stylesheets with more than 4095 selectors, so I don't think there's much value in retaining this test (and its associated functionality). Let me know if anyone disagrees!

Disable sourcesContent generation by postcss
The `standard-minifier-css` package is already associating
source content with the source map, so we don't need to
do this twice.
@hwillson

This comment has been minimized.

Member

hwillson commented Oct 28, 2017

A quick follow up regarding performance benchmarks; This PR introduces postcss as pretty much a drop in replacement for css-parse and css-stringify, which means it has been wired in following the existing API as much as possible. Given this, using postcss is slower in some places, faster in others. Given that postcss can do so much more than the previous libraries, we could likely eradicate any performance differences by re-writing Meteor's CSS build process to better leverage postcss's capabilities. Still though - given everything additional postcss introduces, I'm pretty happy with the benchmark results as is. Here are a few first startup build numbers (taken from a project that has 10,000 lines worth of its own CSS, coming in at about 190K in size):

Using the current css-parse and css-stringify packages:

|    │     ├─ ClientTarget#minifyCss..............................239 ms (1)
|    │     │  ├─ mergeCss.........................................235 ms (1)
|    │     │  │  ├─ CssTools.parseCss                              92 ms (3)
|    │     │  │  ├─ CssTools.mergeCssAsts...........................9 ms (1)
|    │     │  │  │  ├─ CssTools.rewriteCssUrls                      7 ms (3)
|    │     │  │  │  └─ other CssTools.mergeCssAsts                  2 ms
|    │     │  │  ├─ CssTools.stringifyCss                          81 ms (1)
|    │     │  │  ├─ composing source maps                          41 ms (1)
|    │     │  │  └─ other mergeCss                                 12 ms
|    │     │  └─ other ClientTarget#minifyCss                       4 ms

Using postcss:

|    │     ├─ ClientTarget#minifyCss..............................372 ms (1)
|    │     │  ├─ mergeCss.........................................366 ms (1)
|    │     │  │  ├─ CssTools.parseCss                              81 ms (3)
|    │     │  │  ├─ CssTools.mergeCssAsts..........................19 ms (1)
|    │     │  │  │  └─ CssTools.rewriteCssUrls                     18 ms (3)
|    │     │  │  ├─ CssTools.stringifyCss                         196 ms (1)
|    │     │  │  ├─ composing source maps                          55 ms (1)
|    │     │  │  └─ other mergeCss                                 14 ms
|    │     │  └─ other ClientTarget#minifyCss                       6 ms

The above is just a sample run from a checkout using both approaches. The ClientTarget#minifyCss average over 5 runs is much closer:

css-parse and css-stringify approach:

  • Run 1: 239
  • Run 2: 380
  • Run 3: 257
  • Run 4: 250
  • Run 5: 241

Average: 273 ms

postcss approach:

  • Run 1: 369
  • Run 2: 375
  • Run 3: 382
  • Run 4: 349
  • Run 5: 312

Average: 357 ms

@hwillson

This comment has been minimized.

Member

hwillson commented Oct 29, 2017

This should now be ready for review. Quick note - I bumped the minifier-css major version since this work breaks backwards compatibility. I've detailed this in the History.md. I have a feeling the number of people impacted by these changes will be fairly low, as not many people are tying into the minifier-css package directly. These changes might impact build plugin authors that are tying into minifier-css, but then again maybe not (as Meteor's own standard-minifier-css package, that uses minifier-css, isn't impacted for example).

Thanks!

@mitar

This comment has been minimized.

Collaborator

mitar commented Oct 30, 2017

Oh, I just now saw this pull request. This is great and timely work!

I would propose that looking into how to add plugins to postcss should be prioritized higher because juliancwirko's package is not maintained anymore.

We discussed this a bit also in stylus processing pipeline (it has nib and autoprefixer currently) and while you can configure things through addFiles options, there is no way to do so for the whole project.

I would suggest that we simply allow one to put this into package.json, and have postcss configuration there.

@hwillson hwillson changed the title from [WIP] Update the default CSS parsing/combining/minifying tools to Update the default CSS parsing/combining/minifying tools Oct 30, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Oct 30, 2017

Thanks @mitar - the possibility of opening this up to allow postcss plugins is definitely exciting! I just wanted to tackle this in steps, by first introducing postcss to replace the abandoned packages Meteor is using (and to fix issues like #7075). If this PR is accepted then the next step will be to consider opening things up. Actually, I have some quick prototype code ready that does take things further by allowing 3rd party postcss plugins to be leveraged in minifier-css (that I wired up just as a test), and it is working. If we want to go further here (after/if this PR is accepted), I'll definitely look into polishing that code up and submitting a new PR.

hwillson added some commits Nov 1, 2017

@benjamn benjamn added this to the Release 1.6.1 milestone Nov 8, 2017

hwillson added some commits Nov 8, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Nov 8, 2017

@benjamn @abernix I've adjusted the minor versions (of both minifier-css and standard-minifier-css), so this should be all set for a review - thanks!

warnCb(ast.filename, "there are some @import rules those are not taking effect as they are required to be in the beginning of the file");
}
const imports = ast.nodes.splice(0, importCount);
newAst.nodes = newAst.nodes.concat(imports);

This comment has been minimized.

@benjamn

benjamn Nov 8, 2017

Member

Any reason not to do newAst.nodes.push(...imports) here?

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

No reason at all - I'll update, thanks!

// Now we can put the rest of CSS rules into new AST.
cssAsts.forEach((ast) => {
if (ast.nodes) {
newAst.nodes = newAst.nodes.concat(ast.nodes);

This comment has been minimized.

@benjamn

benjamn Nov 8, 2017

Member

And newAst.nodes.push(...ast.nodes) here?

// i.e. //img.domain.com/cat.gif
if (resource.protocol !== null
|| resource.href.startsWith('//')
|| resource.href.startsWith('#')) {

This comment has been minimized.

@benjamn

benjamn Nov 8, 2017

Member

We usually put the || or && operator at the end of the line, so the conditions line up:

if (resource.protocol !== null ||
    resource.href.startsWith('//') ||
    resource.href.startsWith('#')) {

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

Sounds good - I'll adjust, thanks!

// the final resource links (by adding the application deployment
// prefix, here `myapp/`, if applicable).
const relativeToMergedCss = pathRelative(mergedCssPath, absolutePath);
const newCssUrl = 'url(' + quote + relativeToMergedCss + quote + ')';

This comment has been minimized.

@benjamn

benjamn Nov 8, 2017

Member

Unless we really need to preserve the quote character, this can just be

'url(' + JSON.stringify(relativeToMergedCss) + ')'

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

Right, I don't think we need to preserve it, but I'll check (that was old code). If not, I'll update - thanks!

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

I looked into replacing this, but it looks like the current tests go to lengths to make sure the quote is preserved exactly as is. So either no quote, exactly a single quote, or exactly a double quote. There are tests verifying this fairly extensively. I've tested the results of JSON.stringify in practice, and everything seems to be okay, but I'm a bit concerned that the tests were added for a specific reason (maybe browser compatibility issues, special filenames / paths, etc.?). Given this, I'm wondering if we should leave this as is?

*/
minifyCss(cssText) {
const f = new Future;
postcss([ cssnano ]).process(cssText).then(result => {

This comment has been minimized.

@benjamn

benjamn Nov 8, 2017

Member

Can the object returned from postcss(...) be saved for later, or is it important to create a new one each time?

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

postcss() is just a helper function that creates and returns a new Processor object. So it's essentially the same as calling new Processor(). We can definitely save the returned Processor object. It's pretty lightweight so we might not gain much, and we'll have to adjust how CssTools.minifyCss is passing the cssnano plugin into postcss(), but it's definitely doable. I'll adjust - thanks!

This comment has been minimized.

@hwillson

hwillson Nov 10, 2017

Member

I looked into this further @benjamn - unfortunately we can't share the object. In CssTools we're making 3 separate postcss() calls, one of which looks like postcss([ cssnano ]). So here we're passing a plugin into a new Processor call behind the scenes. If we were to change things around so we can share the postcss() created Processor object, I thought we could leverage the Processor objects use method, so we could essentially have something like this._cssProcessor.process() or this._cssProcessor.use(cssnano).process() (where this._cssProcessor points to our shared Processor object). It turns out though that the use method just adds the plugin to the object, so it's then used with any processing from that point forward (with or without the use method). Since we don't always want the plugin, I think our best bet is to leave this as is. Again, the created Processor object is pretty lightweight so this should be okay.

@abernix

Thanks for working on this, @hwillson. Please squash or merge as you feel appropriate.

I believe my vote is on squash, but your call.

@abernix

This comment has been minimized.

Member

abernix commented Nov 15, 2017

(Fixed conflicts in History.md.)

@benjamn benjamn merged commit 8da6c84 into meteor:devel Nov 22, 2017

12 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment