search

hughsk / uglifyify

A browserify transform which minifies your code using UglifyJS2
Other
376 stars 61 forks source link

drive-by fix for minify options brokeness #69

Closed iamjochem closed 7 years ago

iamjochem commented 7 years ago

see #66 for reasoning.

Little Rant: I (& a million others) have spent man-months of wasted time debugging problems related to shitty handling of options/config objects. where "shitty" is defined as applying one or more of the following:

  1. Not cloning [option] objects before passing them to a lib/function.
  2. Not cloning [option] objects when they are passed into your/a lib/function (and then changing the given reference!?!?)
  3. Some module/lib being really helpful and throwing exceptions because "option property 'foo' is invalid/unknown"

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 of uglifyify)

voxpelli commented 7 years ago

This solves the #66 for me

zifeo commented 7 years ago

Also solving #66 in my case. Cheers to @iamjochem!

lrlna commented 7 years ago

Hey! I left a few comments. I am not a fan of Object.assign() tbh, mainly because it's not quite supported everywhere, so I am a bit hesitant. I am down to use extend or xtend, however to create a new object.

iamjochem commented 7 years ago

Object.assign() in node v4+ (not sure down to what version you are supporting) is fine for simple object cloning, but extend() is better here if only because it is doing a deep clone (which even if you don't need it now will save us all another round of time-wasting musical-"options-are-changing-under-my-feet"-chairs.)

iamjochem commented 7 years ago

@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. :-)