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.

Sign up for GitHub

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

Jump to bottom

drive-by fix for minify options brokeness #69

Closed
wants to merge 1 commit into from
Closed

drive-by fix for minify options brokeness #69

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ function uglifyify(file, opts) {
? opts._flags.debug
: true

delete opts._flags

if (ignore(file, opts.ignore)) {
return through()
}
Expand All @@ -37,11 +35,6 @@ function uglifyify(file, opts) {
return through()
}

// remove exts before passing opts to uglify
delete opts.global
delete opts.exts
delete opts.x

return through(function write(chunk) {
buffer += chunk
}, capture(function ready() {
Expand All @@ -51,27 +44,38 @@ function uglifyify(file, opts) {
)

debug = opts.sourceMap !== false && (debug || matched)
opts = extend({}, {

var thisopts = extend({}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to rename this to _opts, so we can distinguish that this is uglify's opts we are gonna use for uglify only. thisopts just seems off for me. but definitely agree that we should create a new obj

Copy link
Author

Choose a reason for hiding this comment

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

this is bikeshedding, no?

sorry, I have lost my sense of humour about this, too much head-banging and time lost due to lazy/dumb/unfriendly option handling (yeah, hi there Babel!)

regardless, I think you should run with your PR's in favour of this one, the end result is the same in terms of fixing the relevant problems/bugs and you seem to be more involved (with the project).

so, by al means delete/close this PR once your's is merged.

sourceMap: {
filename: file
}
}, opts)

if (typeof opts.compress === 'object') {
delete opts.compress._
// remove exts before passing opts to uglify
delete thisopts.global
Copy link
Contributor

Choose a reason for hiding this comment

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

yeeee agree, let's delete these within the stream 👍

delete thisopts.exts
delete thisopts.x
delete thisopts._
delete thisopts._flags

if (typeof thisopts.compress === 'object') {
delete thisopts.compress._
}

if (debug) opts.sourceMap.url = 'out.js.map'
if (debug) thisopts.sourceMap.url = 'out.js.map'

// Check if incoming source code already has source map comment.
// If so, send it in to ujs.minify as the inSourceMap parameter
if (debug && matched) {
opts.sourceMap.content = convert.fromJSON(
thisopts.sourceMap.content = convert.fromJSON(
new Buffer(matched[1], 'base64').toString()
).sourcemap
}

var min = ujs.minify(buffer, opts)
var min = ujs.minify(buffer, thisopts)

if (min.error)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a stream, so we should be emitting an error rather than throwing it (i added this is in #71 when i remapped the argv stuff)

Copy link
Author

Choose a reason for hiding this comment

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

okay, I was aware of what through() was doing, didn't realise that it was generating a stream instance ... although looking at the code I think that due to the implementation of the capture() function that doing throw min.error actually results in an "error" event being emitted.

It is also worth considering that min.error might not be an actual Error instance ... "best practice" states you should only be throwing actual instances of Error (even though you can throw anything you like) .. so we might wish to transform the given min.error into a real Error object (note that min.error may be set by uglyfy's dependencies and that it's type may change depending on what the error is and where it originated.)

throw min.error

// Uglify leaves a source map comment pointing back to "out.js.map",
// which we want to get rid of because it confuses browserify.
Expand All @@ -83,7 +87,7 @@ function uglifyify(file, opts) {

map.setProperty('sources', [path.basename(file)])
map.setProperty('sourcesContent', matched
? opts.sourceMap.sourcesContent
? thisopts.sourceMap.sourcesContent
: [buffer]
)

Expand Down