Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

JS minify failed on public/javascripts/app.js: TypeError: Cannot assign to read only property 'except' of [object Object] #7

Closed
rtruong opened this issue Apr 30, 2013 · 20 comments · Fixed by #9

Comments

@rtruong
Copy link

rtruong commented Apr 30, 2013

When using uglify-js-brunch with the following configuration (from config.coffee):

exports.config =
  plugins:
    uglify:
      mangle:
        except: [ "require" ]

With the following command line:

brunch build --optimize

I get the following error.

error: JS minify failed on public/javascripts/app.js: TypeError: Cannot assign to read only property 'except' of [object Object]

I'm using node v0.10.5, brunch 1.6.3, and uglify-js-brunch 1.5.1.

@paulmillr
Copy link
Member

does uglify works if you do it w/o brunch with these params?

@rtruong
Copy link
Author

rtruong commented Apr 30, 2013

If I invoke uglifyjs with the following command line:

uglifyjs -m -c -r "require" public/javascripts/app.js

It works just fine.

@paulmillr
Copy link
Member

ah, indeed this is the issue, just checked code.

@es128 seems it’s from your changes

@es128
Copy link
Member

es128 commented Apr 30, 2013

I believe I'm just passing through the config parameters as-is. Perhaps these are malformed for what the ugiify API expects? You sure except is a configurable param and expects an array?

@rtruong
Copy link
Author

rtruong commented Apr 30, 2013

If you look at the uglifyjs source, except is configurable and expected to be an array:

https://github.com/mishoo/UglifyJS2/blob/master/bin/uglifyjs#L130
https://github.com/mishoo/UglifyJS2/blob/master/lib/scope.js#L340

@es128
Copy link
Member

es128 commented Apr 30, 2013

The issue is definitely happening within Uglify, though, and you're trying to use the config options in an undocumented way. The only documented usage I can see via the node API is mangle: false. You might need to take your issue over there.

@rtruong
Copy link
Author

rtruong commented Apr 30, 2013

It is not undocumented. Uglify's command line allows for reserved identifiers:

https://github.com/mishoo/UglifyJS2#usage (see -r/--reserved)

And grunt's uglify plugin uses the very same except array:

https://github.com/gruntjs/grunt-contrib-uglify/blob/master/README.md#reserved-identifiers

@rtruong
Copy link
Author

rtruong commented Apr 30, 2013

Moreover, if I hack my own object into uglify-js-brunch/lib/index.js:

  optimized = uglify.minify(data, {
    fromString: true,
    mangle: {
      except: ["require"]
    }
  }).code;

That works just fine. For whatever reason, the config object as it is in the source is causing "TypeError: Cannot assign to read only property"

@es128
Copy link
Member

es128 commented Apr 30, 2013

It's undocumented in terms of the Uglify.minify API method, but yes you've proven your point. I'm glad you were able to isolate the issue further. I'll see if I can figure it out.

@es128
Copy link
Member

es128 commented Apr 30, 2013

Ok, looks like I found the source of the issue and a solution. PR on its way.

@paulmillr Brunch's freeze/deepFreeze stuff has turned out to have lots of side effects. This issue for one, but most notably the watching/changing of config.coffee. How important is it to keep doing that?

@rtruong
Copy link
Author

rtruong commented May 1, 2013

Works fine, now. Thanks for the quick turnaround!

@paulmillr
Copy link
Member

whats up with plugins like that -- sass etc?

@es128
Copy link
Member

es128 commented May 1, 2013

I'll check, but for the most part I don't think they just copy the whole object from config like this one.

@rtruong
Copy link
Author

rtruong commented Jan 31, 2014

Has this issue resurfaced? When using uglify-js-brunch 1.7.4, it works just fine. When I upgrade to 1.7.6, I get the same error I originally reported.

@paulmillr
Copy link
Member

Any points on how can this be reproduced?

@es128
Copy link
Member

es128 commented Jan 31, 2014

Looks like 7b1610a dropped the clone function in favor of a shallow extend. The config object went through deepFreeze, so extend doesn't cut it for solving the same problem as before of uglify attempting to mutate something in the config object that was passed in to it.

@paulmillr
Copy link
Member

extend right now basically copies all properties from config to a new empty object. What's wrong here?

We don't have any nested objects so it should not matter. clone(obj) === extend({}, obj).

@es128
Copy link
Member

es128 commented Jan 31, 2014

We don't have any nested objects

Sure we do. Isn't that the whole source of the problem?

@paulmillr
Copy link
Member

I see. There are arrays. Should be fixed now. @rtruong try master please

@rtruong
Copy link
Author

rtruong commented Jan 31, 2014

Works fine, now. Thanks for the quick turnaround!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants