-
Notifications
You must be signed in to change notification settings - Fork 61
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
drive-by fix for minify options brokeness #69
Conversation
This solves the #66 for me |
Also solving #66 in my case. Cheers to @iamjochem! |
@@ -51,27 +44,38 @@ function uglifyify(file, opts) { | |||
) | |||
|
|||
debug = opts.sourceMap !== false && (debug || matched) | |||
opts = extend({}, { | |||
|
|||
var thisopts = extend({}, { |
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.
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
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.
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.
if (typeof opts.compress === 'object') { | ||
delete opts.compress._ | ||
// remove exts before passing opts to uglify | ||
delete thisopts.global |
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.
yeeee agree, let's delete these within the stream 👍
var min = ujs.minify(buffer, opts) | ||
var min = ujs.minify(buffer, thisopts) | ||
|
||
if (min.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.
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)
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.
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.)
Hey! I left a few comments. I am not a fan of |
|
@lrlna - I rebased my PR to get rid of the conflicts with master but ended up doing this in a fork of my fork (silly rabbit)... which meant I couldn't push the changes ... probably there are git incantations that will fix such a situation but I took the easy/crufty way out and created a new PR. #72 replaces this, so I'm closing it. :-) |
see #66 for reasoning.
1. & 2. are easy to fix -
Object.assign()
is your friend - if everyone would just do this we'd be free of so many bugs related to objects passed in as parameters being changed by the callee.3. - this one requires a change of mindset so that we start treating options object parameters as collections from which known/valid option properties are extracted; this is in constrast to the heavy-handed use of, for instance, JSON schema to validate a given options object and throw an esoteric Error if the options are "invalid" ... which is really helpful when your using a lib indirectly (e.g.
babel
from the context ofuglifyify
)