-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Minify JSON #471
Minify JSON #471
Conversation
src/assets/JSONAsset.js
Outdated
transform() {} | ||
async transform() { | ||
if (this.options.minify) { | ||
this.contents = minify(this.contents, { |
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.
Should check the minify()
result for error
.
https://github.com/mishoo/UglifyJS2/tree/harmony#api-reference
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.
for example:
parcel/src/transforms/uglify.js
Lines 25 to 28 in 68258b1
let result = minify(code, options); | |
if (result.error) { | |
throw result.error; | |
} |
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.
parcel/src/transforms/uglify.js
Lines 25 to 36 in 065a49e
let result = minify(code, options); | |
if (result.error) { | |
throw result.error; | |
} | |
// Log all warnings | |
if (result.warnings) { | |
result.warnings.forEach(warning => { | |
// TODO: warn this using the logger | |
console.log(warning); | |
}); | |
} |
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.
Are we intentionally skipping any custom config for JSON?
parcel/src/transforms/uglify.js
Line 10 in 065a49e
let customConfig = await config.load(asset.name, ['.uglifyrc']); |
Seems a lot of this could be consolidated with the uglify transform.
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.
result.warnings
is not interesting in parsing JS expressions - result.error
is sufficient as it's a binary pass/fail operation. The warnings apply to the uglify compressor which is not applicable here.
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.
@brandon93s i don't think anyone would want to change this behaviour, as it just removing some quotes and whitespace
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.
@kzc i'll throw the errors, forgot about those
Refactored slightly to make JSONAsset not inherit from JSAsset anymore. Seems like that had the potential to break more things than it helped with as we add more stuff to JSAsset that doesn't apply to JSON. |
* uglify-json * throw on error * Clean up JSONAsset Don’t extend from JSAsset
* uglify-json * throw on error * Clean up JSONAsset Don’t extend from JSAsset
closes #400 closes #399