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

Make a copy of incoming options #70

Closed
wants to merge 1 commit into from

Conversation

hershmire
Copy link

Options need to be copied since some properties are later deleted from the referenced object in Uglifyify. If copying does not occur, and the opts object is coming from an assigned variable, that object will be mutated with the deleted properties and any subsequent Uglifyify calls will not have the needed deleted properties – i.e. exts, x, global.

The test is modified to show this case.

@lrlna
Copy link
Contributor

lrlna commented Jun 26, 2017

this is more or less similar to #69 and I kind of like this cleaner approach. What do you think about this @iamjochem ?

@iamjochem
Copy link

my thoughts:

  1. the cloning of the options must occur in the write() function because otherwise two call to ujs.minify() can potentially interfere with each other's options (e.g. the sourcemap related options)

  2. this PR is not deleting the properties on the options object that we know will cause uglyfy to choke ... we not only have to contend with uglyfy mutating the options object it is given but also with the possibility that we are being given an options object that contains properties that uglyfy will choke on.

  3. it is bad practice (IMHO) to re-assign function parameters (last I read V8 refuses to fully optimize functions that reassign their parameters):

function foo(opts) { opts = extend(true, {}, opts); }

should be something like:

function foo(opts) { var _opts = extend(true, {}, opts); }

1. must be changed because otherwise it is still broken (but in an even more subtle way :-())
2. probably needs to change, although I'm not 100% sure this is the right place to do this.
3. not really important: it's some what of a an opinion, v8 best-practices for JS code is a moving target & we're generally talking about running short-lived cmd-line processes (quite likely that optimization for a function that is called just a couple of times does not even occur :-))

@iamjochem
Copy link

PS @lrlna - if you want me to rebase my PR so that it merges cleanly again please let me know

@hershmire
Copy link
Author

Any update on fixing this?

@kumavis kumavis mentioned this pull request Apr 27, 2018
@hershmire hershmire closed this May 6, 2021
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.

3 participants