Skip to content
This repository

Class name mangled after minification #2052

Closed
wuyuntao opened this Issue · 49 comments
Wu Yuntao

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'
Michael Ficarra
Collaborator

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

Kit Cambridge

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.

Michael Ficarra
Collaborator

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

Boris Staal

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.

James MacAulay

My vote goes to __name__

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

benatkin

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

Michael Ficarra
Collaborator

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

Jeremy Ashkenas
Owner

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.

Michael Ficarra
Collaborator

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

Jeremy Ashkenas
Owner

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

Michael Ficarra
Collaborator

Sure thing. I'll get on it.

Devon Govett

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

Jeremy Ashkenas
Owner

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

Devon Govett

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. :(

Michael Ficarra michaelficarra closed this issue from a commit
Michael Ficarra fixes #2052: don't manually assign constructors' `name` property
I'm not sure how we would test this, so... no tests.
f3a1f46
Michael Ficarra michaelficarra closed this in f3a1f46
Michael Ficarra
Collaborator

Fixed.

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

Boris Staal

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

Jeremy Ashkenas
Owner

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 Denicola

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

Michael Ficarra
Collaborator

@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 Denicola

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

Jeremy Ashkenas
Owner

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

Satoshi Murakami
Collaborator
satyr commented

f3a1f46 fixes #2052

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

Marking this wontfix for now.

Michael Ficarra
Collaborator

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

Satoshi Murakami
Collaborator
satyr commented

Er, sorry. I meant #2250 there.

Michael Ficarra
Collaborator

Alright, agreed. This is a wontfix since we're not going to assign a string containing the name to some special property, and f3a1f46 actually fixes #2249 and #2250.

Michael Ficarra
Collaborator

@jashkenas: ping.

Paul Miller

@jashkenas not using any constructor.name analog is a bad desicion.

You may know coffee direction better, but let me explain: there are great real-world use-cases of the feature, backbone classes.

1.3.1 (real production code):

View = require './view'

module.exports = class TweetView extends View
  tagName: 'article'

It will auto-add className: 'tweet' and template: require './templates/tweet'. Convention over configuration, ya know.

Situation on the current master is pretty shitty:

View = require './view'

module.exports = class TweetView extends View
  __name__: 'TweetView'  # etc. No DRY.
  tagName: 'article'

When you were adding the feature, you've said:

Our classes have grown fancier over time. Before they were little more than .prototype = object literal ... now, they're fully executable, with super tricks, static "inheritance" (via copying), named constructors, etc etc. This is just a bit of a further progression towards making them comfy.

and I completely agree with this. Classes allow to build great apps. So why no auto-add @constructor.__name__ etc to them?

James MacAulay

Yay! An implementation :) Thanks @paulmillr!

One of CoffeeScript's biggest advantages is that it fixes a bunch of annoying things about JavaScript: it makes vars always local, equality always strict, switch statements auto-break, etc.

It seems pretty clear-cut to me that Function.name is broken, at least in practical terms. Unless you want to deliver uncompressed JS, you just can't use it at all. And if you tweak your JS compressor to keep function names intact, it ends up resulting in a sub-optimal compression. I see this as the perfect kind of problem for CoffeeScript to provide a solution.

Domenic Denicola

@jamesmacaulay Function.name isn't part of JavaScript; it's a vendor-specific extension. It's outside the scope of CoffeeScript to fix vendor-specific extensions, from what I understand.

Paul Miller

@domenic yea, and we're not fixing them, if we'll use my pull request. We'll just add new functionality. @constructor.name breaks minifiers and is read-only so it's unreliable.

James MacAulay

@domenic fair enough. I'm not arguing that CoffeeScript is "breaking promises" here, though; just that this is a great opportunity to fill an obvious gap.

Wu Yuntao

I totally agree with @paulmillr and @jamesmacaulay.

Class names are widely used in MVC. Model names are used as names of db tables. Controller names are used in URL routing. Views as @paulmillr metioned could generate css classes and template names from their class names.

It could be a great loss if we cannot use them in CoffeeScript classes.

Jeremy Ashkenas
Owner

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

Michael Ficarra
Collaborator

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.

Jeremy Ashkenas
Owner

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.

Michael Ficarra
Collaborator

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.

Jeremy Ashkenas
Owner

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

Would somebody mind uploading this fixed version to npm?

Michael Ficarra
Collaborator

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

Aseem Kishore
aseemk commented

Great points @wuyuntao. I agree wholeheartedly.

( #2052 (comment) )

Alex MacCaw
maccman commented

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.

Marc-André Lafortune

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

Paul Wittmann

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 =(

Jeremy Ashkenas
Owner

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

Marc-André Lafortune

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?

Jeff Dickey

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

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?

Michael Ficarra
Collaborator

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

danschumann

@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?

Dave Jeffery davej referenced this issue in ahmednuaman/radian
Closed

Cleaner controller class syntax #16

tony kerz tony-kerz referenced this issue in michaelficarra/CoffeeScriptRedux
Open

question: will you be re-introducing <class>.name? #302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.