CS2 Discussion: Output: Class Methods aren’t Bound

[ryansolid]

I think this belongs here as it’s more of a directional thing than say a bug. I’ve read jashkenas/coffeescript#4530, but on the surface I didn’t realize how breaking this change is. It’s not just replacing the syntax. The methods aren’t bound. Like pretend you are writing a controller in Express.

class PostsController
  constructor: (app) ->
    app.get '/api/posts', @index

  index: (req, res) -> console.log(@)

@ is now undefined. Now I realize I could have written @index.bind(@). But that is the whole convenience of the Fat arrows. Whole patterns/conventions/libraries in coffeescript are written around this. It’s a set and forget and you don’t have to worry about tracking @. Once you are forcing people to use bind you are back to thinking about tracking @. Now it’s possible I’m not understanding something, but this seems unacceptable from the point of view of the initial goals of this project.

Edit:
While the syntax change could be considered on par with say changing the direction of the spread operator. To me the removal of the ability to bind class methods is more on par with say getting rid of implicit returns. Sure I could go and add return everywhere and rewrite my loops to support it but is that really coffeescript.

Edit2:
I suppose I’m considering this from the perspective of the so called heavy use case. I don’t know how common this is. I learned to use coffeescript this way 5 years ago. In the company I work at pretty much every coffeescript file is a class with bound methods from our components, models, stores, static libraries, and controllers on the web clients, mobile, and the web server as well as a large majority of the files in our backend processing/jobfarms/message queues. This spread over hundreds of modules and hundreds of thousands lines of code. Saying it’s a pain to refactor is an understatement. It basically redefines the language.

Out of the presented solutions making constraints around the use bound methods around super seems like the obvious solution. I say this from the perspective of using both Backbone and React and frameworks which extend base classes in coffeescript as part of the aforementioned code base. It’s possible I am alone on this but given the tools coffeescript gives I find that hard to believe.

[GeoffreyBooth]

So I understand the appeal of being able to just always use =>—like not having to think about var vs let vs const, it’s convenient to never have to consider whether a method should be declared bound or not.

Unfortunately ES2015 classes simply don’t support fat arrow methods. And if we need to choose between supporting the ES2015 class keyword and supporting bound class methods, we need to choose the former for compatibility with other frameworks. In the functional prototype implementation of classes that CoffeeScript 1 uses, it’s not possible to extend an ES class, and that closes the door for using CoffeeScript 1 with certain libraries and frameworks.

So the question is what to do about bound class methods in CS2. This boils down to, essentially, what would you want such code to compile to? What would you want your PostsController example to compile to in ES? And would that ES be equivalent functionally to how the code works in CS 1? Because that was our other concern in deciding to remove bound class methods entirely—if they behave differently in CS2, especially not as people would expect (in our tests, they seemed to behave differently in classes that were extended versus classes that weren’t) then that could be a big potential source of difficult-to-troubleshoot bugs.

[edemaine]

The main suggestion so far has been to put the assignment of bound methods within the constructor. Is compiling to this what you tried in your tests? The other thing I wondered about was replacing the method with a (here famous) get function that would return a bound version of the method. (Haven't tested either of these myself, and how they interact with inheritance.)

[ryansolid]

I would like to give precise numbers on how wide precisely this affects in our code base but I realized it's a bit trickier. Like if we are just talking how often this appears in methods in classes it's not hard for me to estimate it outwards averaging.. I threw a spreadsheet together and got these numbers:

Ultimately skipping deprecated repositories I figure sitting at around 500 classes, with 2700 non-constructor methods with 68% of them that use this. We have several static libraries especially on the server that while written at classes are basically just objects so that lowers the average a decent chunk. We've been trimming the fat, using a lot more 3rd party integrations instead of rolling our own, and migrating to more functional approaches to data mapping instead of declarative nested classes so at it's peak we were easily double that. Obviously there is non-class coffee code like scripts, configuration, data,. So I'm sure in total it makes up for less than 30% of all the coffee code

However as I go I start seeing the ripples. Like use a data-binding framework like KnockoutJS. Well binding say an event handler now needs bind. So that means pretty much updating all the templates. I wouldn't be surprised other databinding frameworks would have this issue.

@GeoffreyBooth asking what I think the output should be is a fair question and I am willing to spend some more time trying inheritance examples etc. I understand why ES classes are so important. Since this change is recent and I've been following as the labelled releases happen I was caught off guard since I didn't realize there was an issue in the past 2 releases where everything seemed to be working great.

[ryansolid]

As far as I can tell this doesn’t work. I’m probably missing something with how the new super works.

class A
  constructor: ->
    @testMethod = =>
      console.log 'BASE'

class B extends A
  constructor: ->
    super()
    @testMethod = =>
      console.log 'CHILD'
      super()

With the nested bound methods in the constructor whats the recommended way to do inheritance/super calls? I tried super.testMethod as well but that doesn’t work either.

[GeoffreyBooth]

@ryansolid When you say you have “2700 non-constructor methods with 68% of them that use this” do you mean 68% are fat arrow methods, or 68% are fat arrow methods that reference @ or this?

If the parent method is a regular class method, it works with super (Try):

class A
  constructor: ->
  
  testMethod: ->
    alert 'BASE'

class B extends A
  constructor: ->
    super()
    @testMethod = =>
      alert 'CHILD'
      super.testMethod()
      
b = new B()
b.testMethod() # Alerts 'CHILD', then 'BASE'

But a method attached to this in a constructor is not a regular class method, and therefore super can’t access it. That’s essentially what you saw in your example.

However, take a look at this example:

class A
  constructor: ->
    @baseTestMethod = =>
      alert 'BASE'

class B extends A
  constructor: ->
    super()
    @childTestMethod = =>
      alert 'CHILD'
      @baseTestMethod()
      
b = new B()
b.childTestMethod() # Alerts 'CHILD', then 'BASE'

So what’s happening here? new B() calls the B constructor, which includes super() that runs the A constructor. At this point, @baseTestMethod gets assigned—but this is the future b object. Then we continue with the B constructor, which assigns @childTestMethod also to this. So now within the B constructor, any bound methods declared within it, or any regular methods declared in the traditional way on class B, both childTestMethod and baseTestMethod are accessed via @.

This also means that if these methods have identical names, the second declaration overrides the first. That’s why in your example, the second @testMethod = obliterated the first one. If you can’t rename one or the other, you could preserve the reference to the base version (Try):

class A
  constructor: ->
    @testMethod = =>
      alert 'BASE'

class B extends A
  constructor: ->
    super()
    @baseTestMethod = @testMethod
    @testMethod = =>
      alert 'CHILD'
      @baseTestMethod()
      
b = new B()
b.testMethod() # Alerts 'CHILD', then 'BASE'

In this version, instead of calling super.testMethod, you would call @baseTestMethod.

I’m sure there are other workarounds. I’m not saying that these patterns are the best way to handle things; I’m sure @connec or someone else who knows classes better than me has better ideas. I just wanted to try to illustrate a bit what’s going on.

Another interesting thing:

class PostsController
  constructor: (app) ->
    app.get '/api/posts', @index

  index: (req, res) ->
    console.log @

app =
  get: (route, fn) ->
    fn()
  
postsController = new PostsController app # Logs undefined
postsController.index() # Logs postsController

Treating @index as if it were a loose function, as you’re doing here in the constructor, does indeed make @ within it undefined. But when calling postsController.index(), @ is the class, as if index was bound. Change @index to @index.bind(@) and they both log postsController.

One last thought: Of the bound methods that reference @, how many reference @ only to call methods on the base class? Any such methods could be refactored to -> functions, with the calls refactored to super.baseMethod.

[ryansolid]

@GeoffreyBooth Thank you. Yeah. I actually I had a reasonable idea what was going on I just was interested to see if there was a proposed approach to it. The problem obviously is if base class methods need to call these methods they can't be named differently.

As for the need to bind @ I agree in many cases when calling off the class it's fine. I was actually auditting that since I was thinking it might not be as big as I thought. But it's looking not to be the case. It's just very common for us to use event handlers this way or data-binding where we are passing a function and we've never had to bind before. It's something we have to be conscious of as the caller instead of as the definer.

---

For my numbers those are fat arrow methods with @ references. We're pretty good at not using Fat Arrows where we don't need them. Where we don't need them we often use static class methods without Fat Arrows or pull the method out of the class and have it only live within the file.

The numbers aren't even really truly representative. I wasn't counting alot of our base frameworks, things we open source etc.. which compile down to javascript at the module level before even being included in the applications. For legacy support reasons and the fact I can ignore them for now I'm not even really putting them into consideration. It might mean the inheritance example I gave in my last post isn't super common, but it otherwise definitely would be. Also there are lot of server Models that have roughly equal number of static and instance methods since they will have both versions of each method (one calling the other) so that we can perform actions with either the instance or just the model id. And I'm also counting classes that basically are all static like a C style library. So basically if you only look under the normal cases where we use instantiated classes it's more like 80% of methods, and like 90% of the class code.

---

Maybe it's better to think of it this way. Consider classes from classic OOP language like C++. Inside the instance method @ is always that instance. These instantiated classes mostly manipulate their internals. One of the appeals of Coffeescript to people would have been that their classes act like C++/Java/C# style classes if you use the fat arrow. It works so well it has been very appealing to just write all the code around it. Classes and their solution via Coffeescript to give that the classic OOP mechanism to Javascript was one of it's initial biggest draws. I'm not coming from the perspective of someone who sees ES6 Classes and thinking we need those (which we do need to support 100%), but rather someone who was sold on the initial sell of adding familiar Class OOP to Javascript.

As for my desired output I'm not sure what's ideal. But almost anything is better than what has been done. I'd just say put it back to how it was before jashkenas/coffeescript#4530. I get the confusing nature of it but the types of mechanism this breaks are ones that largely originated from the lack of classes in Javascript to handle inheritance in the first place. As I mentioned we use Backbone Models. In fact we use them everywhere our ORM is based on Backbone Models. But we've never used initialize. Why? Because Coffeescript has classes. We just do work in the constructor, that's where we set up event handlers etc. Same with React. With Coffeescript 1 although you can't inherit ES6 classes you could use their class style with Coffeescript. Coffeescript's class syntax and patterns have allowed us to make all our code on all platforms and all technologies essentially look the same. When things didn't play nice we forked them, or made local copies converted to Coffeescript (for smaller things).

To me the current solution is way worse that the problem that started it. If you told me I had to rename all my constructors to initialize I'd take that over something so conceptually breaking. This fundamentally changes how we view and use the language. I mean people don't like it don't use it. You can always make your classes without bound methods. But this just destroys a whole paradigm of developing with Coffeescript.

EDIT:
I'm actually curious if people consuming coffeescript are actually happy with the solution(I get it in this case I'm customer to a certain degree, weird to be in the other role, I get the benefits in the simplicity/cleanness). I have to assume the people who originally reported the bug wanted their initialize function to be bound, not that we stop binding everything else. The confusion would have made them angry, but there is a super high chance now they'd have to refactor all the rest of their code if they were using this mechanism already otherwise they wouldn't have found the bug. It's bit of throwing the baby out with the bathwater.

[GeoffreyBooth]

There was a much earlier thread where I proposed separate csclass and esclass keywords, for people to have the option of preserving the CS1 implementation of classes. It was rather roundly rejected, because people using CoffeeScript value simplicity over feature breadth. That’s been a constant throughout the language’s development.

Simply reverting jashkenas/coffeescript#4530 isn’t an option, for the reasons that led to that PR. If you’d like to propose a different solution, please feel free.

One last option is to just keep the CS1 implementation of classes where you need that style:

PostsController = do ->
  ClassPostsController = (app) ->
    app.get '/api/posts', @index

  ClassPostsController::index = (req, res) ->
    console.log @

  ClassPostsController

[ryansolid]

Yeah I understand the reason not to have multiple class keywords. Going to ES5 style and managing your inheritance is obviously pretty bad too and doesn't support extending ES6 classes.

I guess I'm trying to understand how jashkenas/coffeescript#4530 is actually that big of an issue. I'm more saying what led to 4530 should be considered intended behavior. That the solution doesn't fit the problem from a consumer standpoint.

From what I see is the desire to support bound classes with a legacy pattern (setting initializers to be called base class) which is all but being removed from frameworks as they move to class patterns with ES6. And have likely largely already been moved out of Coffeescript implementations if they were using classes. In all cases the people with this issue could always choose not to use bound methods. That prevents the bug. The fact the bug exists is because of the desire to use bound class methods.

From a language design perspective you don't want to have a hole that is inconsistent. However the explanation is simple and not any more confusing than others. Methods aren't bound before their super call. You use that to explain other syntax issues around super. Like why putting @ on a constructor argument it isn't available until after super. You can't enforce it at a compiler level which is the problem.

So I see the rock and the hard place. You are stuck trying to fill the hole. But the solution doesn't even help the user with the initial use case. Pretend you are the user with this issue and you report it. You are clearly using bound class methods. The response says sorry given the restrictions of ES6 classes you can't bind before super. They'd be like that really sucks I have to change all the constructors in all my models/components. And you offer well the alternative is you can't bind anything. What do you think they'd take?

It's obvious from direction this request would even come from that it's a desired feature, if not defining feature of the language. It doesn't take an exercise of pulling out percentages to know that. While I agree not everyone is using the fat arrows in the same way they are definitely using classes with @ references. We may have set the convention for it in very systematic way, but it's always been there to use and people are clearly using it (otherwise it wouldn't be a concern). When talking using client side binding libraries I have to imagine it's ubiquitous.

I guess the problem with 4530 is that I understand the problem (I atleast believe I do). I see the proposed options. The last bullet point seems the obvious choice to me just looking at it. But then there is no discussion no position. No explanation. Just a bunch of yeah sounds good. So I don't feel I understand the reason for the decision because to me in flies in the face of intention of the problem. Which leaves me no forum to even really discuss it.

Edit:
I actually see there is better discussion in jashkenas/coffeescript#4497.

And actually most relevantly here: jashkenas/coffeescript#3093 (comment)

From what I see am I correct that a few higher profile contributors to the project never liked this mechanism and have been looking for a way to remove it for years now because it diverges from JS proper too much as a concept. But until now Jeremy always stated it's usefulness in practice outweighed that.

[ryansolid]

I guess the problem is that this sort of feature is a bit hit or miss on impact. Where people use it, it's more than "irksome" to change how it works. If you made the decision to use this the decision will likely carry through huge portions of your code. So by breaking this functionality might not affect that many people to the same extent but it will alienate certain projects almost completely. I and my team fall into the second category. I might seem dismissive about most of the proposed alternative options, but they don't really work. I don't even have to look far to see why they don't. At a certain point I'm sure enough combinations of them could. It's just rewriting everything at which point you start questioning yourself.

I can respect the difficulty of the decision. Just understand for those affected it may not be minor. I highly doubt that I'm alone in this. But until this enters the more general public perception, you will likely never know. Personally I'd love to really understand how this solution made sense to people, but maybe it's pointless discussing it at this point if it's a foregone conclusion that there couldn't have been a mistake made with jashkenas/coffeescript#4530. To me this feature should have never even been even up for discussion to go on the chopping block in the same way when you took over the project you decided there was going to be no talk of removing implicit returns etc.

As far as I can tell no one had what was considered a "reasonable" solution outside of gutting the whole feature. I think there are only 2 options really the one you took, or leaving it the way it was. There might be small things to aid the latter but that's the fundamental position. I will continue for a bit trying to think of options, but truthfully I lack understanding of the compiler to be able to speak to the part of the issue that I think actually concerns you guys since it was not the use case nor the resultant ES6 that was prioritized here(as both are acceptable with the original code), but rather the implementation. I have nothing there to appeal to you with.

I honestly implore everyone involved to reconsider because I think this will mean leaving it at Coffeescript 1 for a subsection of users. And they may never jump on board again. But I'm pretty sure it was wishful thinking on my part that this migration would be manageable for everyone. The early alpha/betas were really promising. It's just difficult because I feel our investment into really using Coffeescript as Coffeescript right from the beginning is why this is so much harder now. If we hadn't used it for everything, set conventions, trained in it, lived and breathed it this wouldn't have mattered nearly as much.

[GeoffreyBooth]

jashkenas/coffeescript#4497 lays it out. The problem specifically:

Bound methods are now only bound after calling the super constructor. If the super constructor calls any methods in the class, you end up with unbound references to functions.

The most forgiving solution was discussed there:

Rename the bound method and only set the proper name when binding this. Doesn't solve the issue, but would make errors like in the example above more visible. Would need additional thought to avoid naming conflicts and for interaction with non-CoffeeScript classes in the inheritance chain.

So if we had used this solution, you would still need to refactor your code. Any base class constructors that call @ methods would no longer be able to do so. In that thread I think we assumed that that would be a common occurrence, and so this didn’t feel like much of a solution. But who knows, we might’ve been wrong. How often do your base class constructors call @ methods? If it happens rarely, then maybe this might work better.

If you can think of another solution, by all means suggest it. You don’t need to understand the compiler; you just need to understand ES classes. Just provide a few examples of Coffee code with bound classes and what you would want them to compile to in ES. Show how your alternative compilation (as compared to 2.0.0-beta1, the last version that supported bound methods) would avoid the bugs/issues raised by jashkenas/coffeescript#4497. If your ES solves the problem and doesn’t introduce issues of its own, by all means we would go with that instead. Bound methods were removed for technical reasons, not as a design decision. If there’s a way to keep them that isn’t buggy or introduces problems, we will keep them. This is why we have a beta period.

[ryansolid]

Yeah the renaming would require zero refactoring of my code as we don't use that pattern anywhere. The constructor is the thing that already exists to hook into. Other hooks happen later. But I can't speak to everyone. But going forward with what you said I guess ideally something like this:

Coffee:

class A
    constructor: ->
        @baseMethod()

    baseMethod: ->

class B extends A
    baseMethod: =>
        # this works fine
        @callback()
        # the offending call unbound like from an event handler
        @callback.call()

    callback: =>

JS:

checkHelper = (ref, type, fn, args) => {
	if(ref === undefined || !(ref instanceof type))
            throw new Error('Bound class method accessed without being bound in super');
        fn.apply(ref, args);
}

class A {
    constructor() {
        this.baseMethod();
    }
    baseMethod() {}
}

class B extends A {
    constructor() {
        super();
        this.baseMethod = this.__cs_baseMethod.bind(this);
        this.callback = this.__cs_callback.bind(this);
    }
    baseMethod() { checkHelper(this, B, B.prototype.__cs_baseMethod, arguments); }
    callback() { checkHelper(this, B, B.prototype.__cs_callback, arguments); }

    __cs_baseMethod() {
        this.callback();
        this.callback.call(); 
    }
    __cs_callback() { }
}

The checkHelper is really the key to it. If we're ok with runtime errors we might have to throw an exception here to catch the bad usage. This means though that bound methods will have to be associated with their class or inherited class types and you won't be able to bind them to something random. This also is the return of the utility functions you guys were trying to remove from the language. Also if they bind the method to an event handler it won't error out right away until the event fires. So obviously better error and probably better renaming. In cases where a method is inherited from a bound method there is the overhead of the checkHelper because the prototype chain never truly cleaned up, so those undefined/instance checks get called every time you call super if the base class method is bound. But I feel those are reasonable trade offs. I hope the gist of it is clear.

On the positive though this doesn't block inheritance in any sort of way. You can still call bound methods in their parents super in any context that they are still being called with this being the instance (which is the general case). This just catches when they are using it in a way that requires the fat arrow binding before super. It also doesn't break prototype chains with consecutive bound methods in child and parents.

Truthfully this calling methods from the parent constructor is an awkward ES6 problem in general and not just limited to Coffeescript. If you look on stackoverflow: http://stackoverflow.com/questions/32449880/parent-constructor-call-overriden-functions-before-all-child-constructors-are-fi.. Other OOP languages throw compiler warnings about doing it. It just isn't a good pattern for classes. Whether we do something more strict and enforce it is another thing but as you know by now if I had to choose between that and 4530.

[connec]

What if we compiled the prototype version of bound methods to a function that throws an exception, and put the bound function in the constructor (directly or by reference):

class Base
  constructor: (@element) ->
    @initialise()

class Component extends Base
  initialise: ->
    @element.addEventListener 'click', @onClick

  onClick: =>
    console.log 'Clicked!', @

var Base, Component, boundMethodCalledOnPrototype

__boundMethodCalledOnPrototype = function () {
  throw new Error("Bound method cannot be called on prototype")
  // or "bound method not ready" or "method not yet bound" etc.
}

Base = class Base {
  constructor (element) {
    this.element = element;
    this.initialise()
  }
}

Component = (function() {
  var onClick

  // Hoist the function out for readability - we could compile it directly in the constructor as
  // with babel's public class fields plugin.
  onClick = function () {
    console.log('Clicked!', this)
  }

  class Component extends Base {
    constructor () {
      super(...arguments)
      this.onClick = onClick.bind(this)
      // or, as with babel's public class fields compilation:
      // this.onClick = () => {
      //   console.log('Clicked!', this)
      // }
    }

    initialise () {
      this.addEventListener('click', this.onClick)
    }

    // We could even just leave this off, giving people an "undefined is not a function" error
    // instead, but the extra detail would probably help debugging.
    onClick () {
      __boundMethodCalledOnPrototype()
    }
  }

  return Component
})()

This would allow 'legitimate' uses of bound methods and make the calls to the unbound version very obvious. There would certainly be some shenanigans in the compilation, hoisting the function etc. (possibly made easier by the hoisting code added for classes, maybe?).

That said, I still feel that bound methods are a bit of a mistake in the language. Rather than pretend functions work this way in Javascript, I'd rather there was a bound access operator that removes the boilerplate of Function::bind without attempting to change JS semantics. But that said, they have been in the language forever, and clearly appeal to some users in such a big way that removing them is tantamount to removing classes.

Another possible 'solution' to this would be to hold off until we know what we're going to do about property initializers, after which there should be a smoother refactoring path for bound methods (e.g. foo: => to own foo: => or w/e).