-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add process method (compatibility with optimize-css-assets-webpack-plugin) #944
Conversation
Remove unused reject parameter.
@strarsis any updates? |
@jakubpawlowicz: Yes, this can be merged now which |
I meant do you have any comments to my PR review above? ^ |
@jakubpawlowicz: I don't see any PR reviews / file change comments. |
lib/clean.js
Outdated
|
||
// for compatibility with optimize-css-assets-webpack-plugin | ||
CleanCSS.process = function (input, opts) { | ||
return new Promise(function (resolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it somehow use our existing promise interface - https://github.com/jakubpawlowicz/clean-css#promise-interface ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the code is nicer using it.
lib/clean.js
Outdated
return new Promise(function (resolve) { | ||
var cleanCss = new CleanCSS(opts); | ||
var output = cleanCss.minify(input); | ||
resolve({ css: output.styles }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question here - what happens with other output
fields, like errors
, sourceMap
, stats
, or warnings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The webpack plugin only expects/uses the css property of returned object: https://github.com/NMFR/optimize-css-assets-webpack-plugin/blob/master/index.js#L78
Ah sorry, forgot to submit the review. |
lib/clean.js
Outdated
return cleanCss.minify(input) | ||
.then(function(output) { | ||
return { css: output.styles }; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Please use 2-space indent in lines 54-56 to be consistent with other code here and I'll merge it, thanks!
Extra note: optimize-css-assets-webpack-plugin passes the |
In case of clean-css that |
Option |
👍 somehow indents are wrong again, please format it as: return cleanCss.minify(input)
.then(function(output) {
return { css: output.styles };
}); |
@jakubpawlowicz: Fixed |
Thanks! |
I wasn't able to get this working at all, let alone with source maps. Instead I had to put together something a bit custom to get it working and with source map support. Might be useful for someone in the future: const path = require('path');
const OptimizeCSSPlugin = require('optimize-css-assets-webpack-plugin');
const CleanCSS = require('clean-css');
...
plugins: [
// Compress extracted CSS. We are using this plugin so that possible
// duplicated CSS from different components can be deduped.
new OptimizeCSSPlugin({
// use clean-css instead of the default cssnano
cssProcessor: {
process: (css, options) => new Promise((resolve, reject) => {
const opts = Object.assign(options.map ? {
sourceMap: true,
sourceMapInlineSources: true,
} : {}, options);
opts.returnPromise = true;
const filename = path.basename(opts.to);
new CleanCSS(opts)
.minify({
[filename]: {
styles: css,
sourceMap: opts.map && opts.map.prev, // probably empty but here just in case
}
})
.then((result) => {
if (result.warnings) console.warn(result.warnings);
resolve({
css: options.map
? result.styles + `\n/*# sourceMappingURL=${filename}.map */`
: result.styles,
map: result.sourceMap,
});
})
.catch(reject);
}),
},
cssProcessorOptions: {
// passing an object will enable source maps OR comment out to disable
map: {},
// the actual clean-css options
level: {
1: { all: true },
2: { all: true },
},
},
}),
]
... |
@jakubpawlowicz Has this been published to the npm? |
@yshterev same to you, it doesn't work. : ( |
It can be used like this until it is fixed:
|
still an issue |
This PR adds the process method to clean-css so
optimize-css-assets-webpack-plugin can use it as css processor.
Closes issue #943.