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

Class name mangled after minification #2052

Closed
wuyuntao opened this issue Jan 18, 2012 · 52 comments
Closed

Class name mangled after minification #2052

wuyuntao opened this issue Jan 18, 2012 · 52 comments
Assignees
Labels

Comments

@wuyuntao
Copy link

@wuyuntao wuyuntao commented Jan 18, 2012

JavaScript minification tools rename local functions for performance, and this mangles CoffeeScript class names too.

Since Function.name is a read-only property, forcing class names which implemented in #494 does not work.

How about setting class name to some other property, like Foo.__name__ or Foo.className?

A brief example

class Foo
  constructor: ->
    console.log(this.constructor.name)

foo = new Foo()

Compiles into

(function() {
  var Foo, foo;
  Foo = (function() {
    Foo.name = 'Foo';
    function Foo() {
      console.log(this.constructor.name);
    }
    return Foo;
  })();
  foo = new Foo();
}).call(this);
// Outputs 'Foo'

Minified

(function(){
  var a,b;
  a=(function(){
    a.name='Foo';
    function a(){
      console.log(this.constructor.name);
    }
    return a;
  })();
  b=new a();
}).call(this);
// Outputs 'a'
@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jan 19, 2012

I would be in favour of Foo.name = Foo.displayName = "Foo" compilation.

@ghost
Copy link

@ghost ghost commented Jan 19, 2012

Something to consider: (function self(){ "use strict"; self.name = self.displayName = "foo"; }()) will raise a TypeError. I don't think there are too many cases where compiled CoffeeScript code would be combined with strict mode JS code, but it's something to be aware of.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jan 20, 2012

@kitcambridge: Huh. That's interesting, since we're already assigning the name property currently. Maybe we should just change it to Foo.displayName = "Foo" then.

edit: We could additionally include if(!__hasProp(Foo, 'name')) Foo.name = "Foo";, but I don't think I would support that.

@inossidabile
Copy link

@inossidabile inossidabile commented Jan 26, 2012

Assigning .name property would be very confusing since this leads to different behavior in IE and other browsers. It's especially notable when everything stops working right after auto-minification on the production.

IMHO the current fix on #494 make the situation way worse then it was before. With that you have even more chances to stuck into reality much too late.

My vote goes to __name__. I trust you should not pollute base class naming. I can easily imagine "displayName" as a field I'd like to use myself. Same goes to className. __notation__ is well known for such kind of hacks and will do the job very well.

@jamesmacaulay
Copy link

@jamesmacaulay jamesmacaulay commented Feb 8, 2012

My vote goes to __name__

+1. Or possibly just __name, to go along with __extends et al.

@benatkin
Copy link
Contributor

@benatkin benatkin commented Mar 11, 2012

@jamesmacaulay Good thinking, but I think it goes along more with __super__ than with __extends, so it should be called __name__. Though __name would probably have less chance of conflict...

@ghost ghost assigned jashkenas Mar 11, 2012
@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Mar 14, 2012

@jashkenas: do you have enough information to make a decision for us here?

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 11, 2012

Ugh -- this is bad news. I now wish I had stuck to my guns on #494, and left forced class names out. Let's remove them at our nearest convenience.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 11, 2012

@jashkenas: Do you want to define an alternative property or just remove it entirely?

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 11, 2012

I'd like to remove it entirely. If you want to tag a name on your class -- tag a name on your class yourself.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 11, 2012

Sure thing. I'll get on it.

@devongovett
Copy link

@devongovett devongovett commented Apr 11, 2012

NOOOOOOOOO! Such a useful feature! Can't we just go with .displayName or something else? :) displayName is already used by developer tools to display names of functions in the profiler, and I'm pretty sure other langs like Objective-J compile that too... Pretty please? :P

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 11, 2012

Nope. You can already see the name of the function in any decent debugger.

@devongovett
Copy link

@devongovett devongovett commented Apr 11, 2012

Probably. But I use the name feature for other reasons, as detailed in #494 by myself and @hornairs such as automatically figuring out the REST url for a resource from the class name. Pretty soon we're gonna have to start writing extensions to CoffeeScript to get the useful features that keep getting rejected, like this and the extended hook. :(

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 11, 2012

Fixed.

@jashkenas: It seems like this issue is affecting a lot of our users. Time to cut another bugfix release?

@inossidabile
Copy link

@inossidabile inossidabile commented Apr 11, 2012

Ah :(. Really bad news. We've been waiting for this so much time and now it will never happen 😱

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 11, 2012

It seems like this issue is affecting a lot of our users.

... I wasn't getting that sense -- I think it's only a problem if you're intentionally strict-mode-ing your CoffeeScript ... which you certainly don't have to do. Let's wait a reasonable interval before the next bugfix point release.

@domenic
Copy link
Contributor

@domenic domenic commented Apr 11, 2012

Just lending my support to backing out the assignment. Function#name already has a meaning, and messing with it is a bad idea.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 11, 2012

@jashkenas: See #2249, #2250. Within 24 hours, we've already had 2 issues opened. At this rate, we'll have over a dozen by next week :trollface:.

@domenic
Copy link
Contributor

@domenic domenic commented Apr 11, 2012

Indeed, new release please; cannot upgrade until this is fixed. Another example.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 11, 2012

Let's shoot for a stable one tomorrow afternoon then.

michaelficarra added a commit that referenced this issue Apr 11, 2012
@satyr
Copy link
Collaborator

@satyr satyr commented Apr 11, 2012

f3a1f46 fixes #2052

That fix is for #2050#2250, which isn't really a duplicate of this.

Marking this wontfix for now.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 12, 2012

@satyr: #2050 is a pull request for line mappings. Now I'm confused.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 16, 2012

@michaelficarra -- What's the "ping" regarding? Aren't we settled on this?

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 16, 2012

Let's shoot for a stable one tomorrow afternoon then.

But it seems now that a lot of people don't want the change that's currently on master, so it's your call. I think @paulmillr's pull request is reasonable.

edit: Oh, it looks like you just turned that down. We should push out another bugfix release, then.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 16, 2012

Let's shoot for a stable one tomorrow afternoon then.

Hah. Got away from me...

I really don't want to mint new "name" semantics that are special for CoffeeScript classes. I think it'll end in tears.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 16, 2012

That's entirely reasonable. Are you planning on pushing the release that removes the name assignment soon? There's 1 or 2 other fixes on master as well.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 16, 2012

Yep -- and maybe bringing a few other tickets along for the ride. I'm glad that nothing else pressing has come up over the weekend, apart from "use strict".

@bennedich
Copy link

@bennedich bennedich commented Apr 28, 2012

Would somebody mind uploading this fixed version to npm?

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Apr 28, 2012

@bennedich: as soon as a new release is pushed. See #2282.

@aseemk
Copy link
Contributor

@aseemk aseemk commented May 14, 2012

Great points @wuyuntao. I agree wholeheartedly.

( #2052 (comment) )

@maccman
Copy link

@maccman maccman commented May 15, 2012

Agree with @wuyuntao - class names are incredibly useful (used all over the place in Spine, for example). The only issue here is that the standards committee made the wrong call by making .name readonly. I think .displayName would suffice, and wouldn't necessarily lead us down a slippery slope of polluting classes.

@marcandre
Copy link

@marcandre marcandre commented Jun 3, 2012

Add me to the camp of "give me access to my class name". I like constructor.className as it is very descriptive.

@paulkoegel
Copy link

@paulkoegel paulkoegel commented Jul 29, 2012

very confused about the status of this. is it still a wontfix or has it already been fixed as jeremy and paul's comments here (#2262) seem to suggest?
i'm using rails 3.2.6 with coffee-script-2.2.0, coffee-script-source-1.3.3, and coffee-rails-3.2.2 - and still get .name errors when using strict mode =(

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jul 30, 2012

CoffeeScript has removed the additional setting of the .name property on classes ... because of strict mode.

@marcandre
Copy link

@marcandre marcandre commented Aug 15, 2012

CoffeeScript has removed the additional setting of the .name property on classes

Any plan to add constructor.className or similar? Are there reasons against it?

@jdickey
Copy link

@jdickey jdickey commented Aug 15, 2012

Certainly on our project, it would simplify some otherwise-gnarly code dramatically; I'm sorry, but the tediousness and likelihood of errors scales up far more rapidly than the number of classes involved in a non-trivial hierarchy. If you're being a Good Little Scripter™ and making extensive use of private vars/methods, sticking an extra, public "Hi! My Name is FooClass!" sticker on to cover a weakness in the language just Feels Very, Very Wrong™. And it really doesn't matter if the "weakness" is in CoffeeScript's layer or the underlying JS-in-strict-mode; enough people have expressed enough pain induced by this that we need to find a solution. Our project's "workaround" looks to be moving as much current front-end Script functionality as possible to the back-end Ruby code and just hitting the network much more than we are now, which has implications that should be quite foreseeable. I'm already short two developers; we don't have the time to grok the source and fix it ourselves, and even if we did, our fix would be too unlikely to be compatible with what @jashkenas or others in the community come up with simply because we don't have the same level of expertise.

So where do we go from here?

@danschumann
Copy link

@danschumann danschumann commented Nov 13, 2012

At some point coffeescript will become too big to please everyone, right? Well, it's probably too late for that already. What about a config file that allows you to toggle on or off some options like this one, since I think a lot of people are for it, even if a lot are against it, why not make that a simple true false in a compiler.conf? I'm on the side for it by the way, since I don't want minifying my code to break any lazy references I've made to classes, and developers here are lazy and referencing classes in such a way, and I don't blame them. If you make a class, that class should know what it is made of in a reliable fashion. I have 98 more cents to spend, anyone have any gum?

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Nov 13, 2012

@danschumann: This is no longer a problem when using CoffeeScriptRedux's source maps.

@danschumann
Copy link

@danschumann danschumann commented Nov 13, 2012

@michaelficarra So.. what's with the original coffeescript then? Is it moving to become the redux coffescript 2, or are they both being maintained in parallel? Is it easy to use the customization options? The readme on cs2 doesn't seem to say anything.. what's the difference and what do I have to read?

vitalyiegorov pushed a commit to samsonos/php_coffeescript that referenced this issue May 11, 2014
"Due to the new semantics of JavaScript's strict mode, CoffeeScript no longer guarantees that constructor functions have names in all runtimes. See #2052 for discussion."

jashkenas/coffeescript#2052
trabianmatt added a commit to trabian/trabian-webapp-core that referenced this issue May 19, 2014
I was seeing some odd behavior when using a minified version of the js
on a project that leveraged the IdentityMap. It was caused by the use of
the class as the cache key -- js hashes only support string-based keys
so the toString() of the class constructor was being used as the hash
key.

This was fine (though inefficient) until the minification tool reused
function names when they weren't in the same scope (which is fairly
common when using CommonJS). A similar issue was covered in
jashkenas/coffeescript#2052.

This was solved in our case by assigning a unique CACHE_KEY property to
the class the first time it requests its cache. **Be aware, however,
that if you extend a class that already has a CACHE_KEY then it will use
that existing class's CACHE_KEY and therefore will mix IdentityCache
entries.** You can explicitly add a CACHE_KEY class property to avoid
this situation.
@CyborgMaster
Copy link

@CyborgMaster CyborgMaster commented Mar 16, 2015

Sorry to bump such an old thread, but I had a few questions. I am working on a project where we need to get a hold of the class name. We are building some features into our models for auto-configuration (convention over configuration), where the names of id properties, json urls, etc. are intuited from the model class name (similar to what Rails does).

What is the current official recommendation from coffeescript on how to do this? I loved the name property that was added to constructors, but based on this thread, that seems to have been removed. I also loved the idea of __name__ to handle minification and browser support issues, but that also seems to have been rejected.

Is the current recommendation to simply add another property on the class manually? Something like this?

class Foo
  @name: 'Foo'

That works, but it certainly isn't very DRY.

If the manual class property is all we've got then I will use it of course, but I just wanted to check what the official recommendation was.

Sorry if this was the wrong place to ask. I thought about opening a new issue, because this one is so old, but it seemed like the correct thread. If there is a better place to ask let me know and I can repost and do better in the future.

Thanks!

@epidemian
Copy link
Contributor

@epidemian epidemian commented Mar 17, 2015

@CyborgMaster you can assign the class to some object's property:

class Models.Cat
  talk: -> 'meow'

# Or...
Models.Cat = class 
  talk: -> 'meow'

# Or...
@Cat = class
  talk: -> 'meow'

That way, you can trust that the name will be preserved upon minification because you've assigned it to a named property on an object, and not just a local variable.

Don't take this for an "official recommendation" though. It's just what i would do to keep a mapping between classes and some names. I hope it helps!

@davidbonnet
Copy link

@davidbonnet davidbonnet commented May 31, 2015

If someone else is stumbling on this, here are three solutions:

  1. Don't rely on Function::name at all, or...
  2. Use uglify with the --keep-fnames flag to leave function names untouched, and...
  3. Add support of the Function::name property in IE, with the following snippet:
unless Function::name?
    Object.defineProperty Function::, 'name',
        configurable: yes
        get: ->
            name = ( @toString().match /^\s*function\s*(\S*)\s*\(/ )[1]
            Object.defineProperty @, 'name', value: name
            return name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.