constructor renaming parameters

[crisward]

If you do this

class Blah
  constructor:(@thing)->
     @thing()
      
  method:->
    thing = "hi"

Coffeescript compiles to

var Blah;

Blah = class Blah {
  constructor(thing1) {
    this.thing = thing1;
    this.thing();
  }

  method() {
    var thing;
    return thing = "hi";
  }

};

Why the 1 after the thing?? I'm using an dependency injection system, and the rename is playing havoc with it. Coffeescript 1 didn't do this.

[vendethiel]

Coffeescript 1 didn't do this.

It's been doing this since 1.9.0, 29jan 2015.

[crisward]

Is there any documentation or issue as to why coffeescript does this? I can understand why it may be necessary if the variable is declared outside the class, but inside the function doesn't seem to make any sense.

[lydell]

@crisward Why does it matter to you what a generated variable name looks like? Could you show an example?

[vendethiel]

8ab15d7

@lydell probably the DI system looks up the name of the parameter to inject. I know AngularJS1 did that.

[crisward]

We use a dependency injection system which parses the output of the constructor to know what modules to inject (a bit like Angularjs did). This can't find a module with a 1 at the end.

[crisward]

Just checked and coffeescript 1.9.1 didn't do this.

[lydell]

Just search for “angular” in this issue tracker and you’ll find everything you need.

[vendethiel]

➜  coffeescript git:(533ad8a) ./bin/coffee -bce '(@a) -> a'
// Generated by CoffeeScript 1.9.1
(function(a1) {
  this.a = a1;
  return a;
});
➜  coffeescript git:(533ad8a) ./bin/coffee -v
CoffeeScript version 1.9.1

CoffeeScript 1.9.1 did do that.

[vendethiel]

Also, how does your DI deal with minified code? I know that was an issue in AngularJS and that's why people ultimately stopped doing it (... or developed modules to do it automatically).

[crisward]

It's node, so doesn't need to be minified. Also you can just not mangle in uglify for client side stuff.

I have a codebase I'm converting from 1.9.1 to 2.* and it's breaking in lots of places. So there is a difference somewhere. I'll take a look around the issues and see if I can get a work around. The other option I have is to avoid using a lib name in my code, just seems a bit unnecessary.

BTW thanks for the rapid response.

[vendethiel]

Just to be clear: if you write (@a) -> @a, then the compiler doesn't have to reserve a, and names the parameter a. I would recommend against depending on this, but it should solve your outstanding issue.

[lydell]

@vendethiel That is super fragile though becase if you name a variable a anywhere else in the same file (even if not in the same method) the generated variable will become a1.

[crisward]

@vendethiel thanks for the help, I looked at some of the other issues. I'm injecting @request, and using the variable name request in a method, which causes @request to become @Request1.

I think the simplest solution as I'm migrating old code is just to add an alias to my injector for generic library names to be added with the additional '1' on the end. Request is the main one as it's a commonly used variable name in node code.

Thanks.

[vendethiel]

@vendethiel That is super fragile though becase if you name a variable a anywhere else in the same file (even if not in the same method) the generated variable will become a1.

It definitely is. That's why I recommend against it. It's just to help migrate code.

[lydell]

@crisward It is not necessarily a 1 at the end. It could be larger numbers, too, depending on your code.

A more robust solution is:

class A
  m: (a) ->
    @a = a

But you’ll probably get away with the 1 check, too I guess.

[crisward]

If I had to do that, I'd probably just give up and use plain js 😢

[connec]

I seem to recall that when this change was made it was reduced to only cover cases where the same name is used within the function (e.g. (@a) -> a)? It looks like there might be a scoping issue, I can't see why constructor's scope should be influenced by method's scope.

[lydell]

@connec The code simply gets a list of all used identifiers in the whole file and then avoids using those for generated variable names. Why? Because it’s simple. Tracking scope is more complicated for no gain. And even if we did that it wouldn’t help if the name is in the same scope.

[connec]

I see, we only keep track of assignments in Scope, not usage, I guess that makes sense.

[crisward]

In my example the variable name is not in the same scope, but I understand the need for simplicity.

[phil294]

Hi @GeoffreyBooth, maybe this should be reopened: The above comments said narrowing down the scope tracking would be unnecessary. However, @robert-boulanger brought up a very valid use case, explained here: JSDoc.

for example

class ClassTest
  ###*
   * @param {object} option
  ###
  constructor: (@option,@callMe, name)->

option = 1

@option works with option1, so the comment block becomes spaghetti.

[edemaine]

I'm working on a fix to this (it's relatively simple), but am running into an issue with the current codebase: traverseChildren with a true first argument (crossScope) causes an infinite loop. I can try to track down where the issue is (maybe a loop in the children pointers?), but if anyone knows why this might be happening, let me know.

Update: the infinite loop is caused by tests failing in a bad way; never mind!

[lydell]

What are you going to do in this case?

option = 1

class ClassTest
  ###*
   * @param {object} option
  ###
  constructor: (@option,@callMe, name)->
    console.log("sum", @option + option)
    
new ClassTest(5, "", "") # Should log `sum 6`

[edemaine]

@lydell In that case, my code uses option1. You still need to avoid name conflicts with descendants, which is easier to control; conflicts outside this scope are ignored.

[edemaine]

For those following this issue, #5398 is the code I was describing above, which I believe fixes this issue where possible.