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.

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 (replacement for PR#69) #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamjochem
Copy link

@iamjochem iamjochem commented Jun 26, 2017

this is a replacement PR for #69 ... (also 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)

@hershmire
Copy link

Any update on when we can get a fix merged in?

@mwiencek
Copy link

mwiencek commented Sep 8, 2017

I upgraded uglifyify from 3.0.1 to 4.0.3 and it stopped minifying my code completely (no visible errors). Applying this patch fixes it. Can it be merged?

@kumavis kumavis mentioned this pull request Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants