CS2 Discussion: Output: Class Methods aren’t Bound

[coffeescriptbot]

From @ryansolid on 2017-05-18 08:50

I think this belongs here as it’s more of a directional thing than say a bug. I’ve read https://github.com/jashkenas/coffeescript/pull/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.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-18 18:19

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.

[coffeescriptbot]

From @edemaine on 2017-05-18 23:12

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

[coffeescriptbot]

From @ryansolid on 2017-05-19 04:38

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.

[coffeescriptbot]

From @ryansolid on 2017-05-19 05:11

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.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-19 07:01

@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](http://coffeescript.org/v2/#try:class%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%0A%20%20testMethod%3A%20-%3E%0A%20%20%20%20alert%20'BASE'%0A%0Aclass%20B%20extends%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%20%20super()%0A%20%20%20%20%40testMethod%20%3D%20%3D%3E%0A%20%20%20%20%20%20alert%20'CHILD'%0A%20%20%20%20%20%20super.testMethod()%0A%20%20%20%20%20%20%0Ab%20%3D%20new%20B()%0Ab.testMethod()%20%23%20Alerts%20'CHILD'%2C%20then%20'BASE')):

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](http://coffeescript.org/v2/#try:class%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%20%20%40baseTestMethod%20%3D%20%3D%3E%0A%20%20%20%20%20%20alert%20'BASE'%0A%0Aclass%20B%20extends%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%20%20super()%0A%20%20%20%20%40childTestMethod%20%3D%20%3D%3E%0A%20%20%20%20%20%20alert%20'CHILD'%0A%20%20%20%20%20%20%40baseTestMethod()%0A%20%20%20%20%20%20%0Ab%20%3D%20new%20B()%0Ab.childTestMethod()%20%23%20Alerts%20'CHILD'%2C%20then%20'BASE'):

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](http://coffeescript.org/v2/#try:class%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%20%20%40testMethod%20%3D%20%3D%3E%0A%20%20%20%20%20%20alert%20'BASE'%0A%0Aclass%20B%20extends%20A%0A%20%20constructor%3A%20-%3E%0A%20%20%20%20super()%0A%20%20%20%20%40baseTestMethod%20%3D%20%40testMethod%0A%20%20%20%20%40testMethod%20%3D%20%3D%3E%0A%20%20%20%20%20%20alert%20'CHILD'%0A%20%20%20%20%20%20%40baseTestMethod()%0A%20%20%20%20%20%20%0Ab%20%3D%20new%20B()%0Ab.testMethod()%20%23%20Alerts%20'CHILD'%2C%20then%20'BASE')):

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.

[coffeescriptbot]

From @ryansolid on 2017-05-19 15:38

@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 https://github.com/jashkenas/coffeescript/pull/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.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-19 17:27

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 https://github.com/jashkenas/coffeescript/pull/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

[coffeescriptbot]

From @ryansolid on 2017-05-19 18:21

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 https://github.com/jashkenas/coffeescript/pull/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 https://github.com/jashkenas/coffeescript/issues/4497.

And actually most relevantly here: https://github.com/jashkenas/coffeescript/issues/3093#issuecomment-21745789

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.

[coffeescriptbot]

From @ryansolid on 2017-05-19 19:10

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 https://github.com/jashkenas/coffeescript/pull/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.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-19 19:41

https://github.com/jashkenas/coffeescript/issues/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 https://github.com/jashkenas/coffeescript/issues/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.

[coffeescriptbot]

From @ryansolid on 2017-05-20 06:25

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.

[coffeescriptbot]

From @connec on 2017-05-20 12:12

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

[coffeescriptbot]

From @ryansolid on 2017-05-20 13:59

@connec I have to imagine Typescript must do their bound methods something similar given the constraints they have around theirs. It's a reasonable proposal. It does deal with where most of the pain comes from in practice. Although more than anything it actually showed me why my example above (the arguably simplest case) isn't actually the complete picture. If I was truly picking an example from the code at my office (although we don't use this pattern of calling in the constructor), the idiomatic definitions would look more like this:

class A
    constructor: ->
        @baseMethod()

    baseMethod: =>
        # do some stuff with @

class B extends A
    baseMethod: =>
        super(arguments...)
        @addEventListener('click', @callback)

    callback: =>

Although we don't inherit these functions called in constructors as the example we definitely inherit bound methods. I don't believe the hoisting as presented let's you do that. You wouldn't be able to call super on baseMethod as it would find the prototype method and throw the error. Although what I posted does allow that. Obviously this example will throw the error for the callback in both cases and not until the event is fired.

I will take some time to read about property initializers. If they support inheritance I think that could be reasonable way forward. As much as the lack of ability to work with ES6 classes is very painful if delaying the migration by a couple years would make a big difference I suppose it could be workable. It wasn't that long ago that this wasn't even really hope, and have stuck with it this long. If there was a way to keep classes as close to their CS1 brethren obviously that would be the best from my perspective. Atleast until there is a better option (although I do see the desire to start CS2 clean if there is a better long term solution).

[coffeescriptbot]

From @connec on 2017-05-20 15:58

I see, your version would indeed catch more access patterns.

Whilst I'm not a fan of bound methods, I feel your pain wrt conforming to ES2015's class semantics. You may find some of the threads in the ES2015 class PR interesting for reference. There were already a few hoops jumped to support bound methods and @params, along with a few other subtleties (e.g. mandatory super etc.).

I take it, for your purposes, a bound access operator would not be ideal? I imagine the refactoring effort of finding and updating every call site where the bound-ness matters would be a significant undertaking.

[coffeescriptbot]

From @ryansolid on 2017-05-20 19:19

Yeah I mean I think what you guys have done given what the undertaking is fairly elegant. While it forces some restrictions it doesn't muddy the code or even really the output code.

Yeah bound accessors changes where the decision happens which isn't particularly helpful. Property initializers are alright although if we are thinking of this as a direction they are much closer to the hoisting into the constructor.

I think the last few years while ES6 has been getting accepted through Babel in live frameworks and the growth of maturity in those frameworks the perception around Javascript has changed a lot since Coffeescript was released. Coffeescript while always just compiling to Javascript sort of flew in the face of it a bit. And now several years later we're stuck recognizing that Javascript is the direction of Coffeescript's future. In that perspective I think that actually limits what Coffeescript can and should be doing. Bound class methods don't make sense for JS and didn't make it into ES Classes. Coffeescript used to be able to say I don't care, let's do the conceptually simpler and more familiar thing (from other OOP languages).

In terms of your proposal (to hoist them) for bound methods might actually be enough in practice. If I give up that desire to pretend Javascript has something it does not (no matter how appealing it is) there are some things that could be workable. I need to try a few things over the next few hours but my thoughts are the following:

1. Even if a class method accesses @ it isn't as nearly as common that it needs to have a Fat Arrow since it will never be used that way. Also the decision to be bound is always made to it directly. It's not like it gets added to another handlers callback necessitates it's own binding. So you can address at the time of binding. So that being said .....
2. In most cases methods passed around to handlers aren't overridden and not in the sense of calling super. Inheriting Bound methods may not be actually necessary. The decision to be bound still can belong to the definer not the caller.
3. Hoisting the methods is conceptually similar to that proposal for property initializers. Even if that never goes through or the whole landscape 180s, it is similar to what Typescript does to support bound class Methods and it isn't that far of a departure from the current behavior sans inheritance. It covers the 90% case.

I know that I definitely have code that doesn't follow the general case of point 2. Pretty much all my REST Controllers. But they aren't deeply inherited and will never be so a change in the library base class can do a lot of the heavy lifting. Most importantly is code could be migrated to this paradigm of Fat Arrow and still work in CS1 I believe. I haven't been concerned so much about the migration at a file scope. More that it'd be immediately broken in a way that would mess up everything downstream. Something like forcing arguments in super is manageable since even if I have to update that 1200 times I don't need to code split the modules since if only one application is building in CS2, the modules still build in CS1 for everything else. I'm going to have to hit everyone of those files, but it can be a modular systematic process and one I can start before committing to migrate. Removing unnecessary Fat Arrows is similar.

So I'm going to validate this approach you have suggested. Obviously my proposal is the closest to make it work exactly like CS1 classes since I even allow bound methods in the constructor as long as they aren't being used out of their instance context. This keeps maximum compatibility for bound methods and only errors out where we absolutely have to. Inheritance fully works. So it feels like all the benefits of CS classes and ES classes. I think there is a lot appealing about that (besides the fact it would require no refactoring for me). However I can see this JS purist, pragmatic perspective, that we're hacking something unnecessary on top that can never be without holes and no matter how well documented or how well handled will confuse someone. That's a tough one guys. Obviously there is the whole side of validating that what has been proposed even works completely but directionally that's hard. And I recognize there are biases, although I applaud everyones open-mindedness to atleast recognize their perspective might not be best for the language and community on a whole.

EDIT: I also recognize at a certain point with hoisting as a language construct, can be deflected back to just do that yourself and leave it at no Bound Class Methods. So I have to admit I have a least a little syntax prejudice. Beyond it being more refactoring work that replacing an = sign with a - in most cases and that I can change them at my leisure as well since they will only break in very specific cases. Guys it's awkward and ugly for something would be almost used exclusively in the client. I realise property initializers gain that back but that's not a migration path and there is uncertainty. At a minimum I think making it part of the language is necessary today.

EDIT2: With only addressing a small test web app with endpoints (yet to try React Native mobile) using most of the pieces @connec suggestion can work pretty well. The saving grace is a lot of our open source libraries that we write/contribute to are compile down to JS. So while conceptually looking at the source I could see places it would require more work if they interacted directly it didn't matter. Application level code never went further than 2 inheritance levels on top that. It helped that most of the client code needed to Fat Arrows so there wasn't much to change. As mentioned whether that's the right decision or not is a different thing. But if that was direction this went I couldn't really be opposed from a refactor and on the surface development experience perspective. Preferred not, but nothing to complain about. I guess in both cases it needs to be vetted any gotchas.

[coffeescriptbot]

From @ryansolid on 2017-05-23 18:56

So where do we go from here? I think mine or @connec approach would work for the majority of users for the majority of their usage. But I'm pretty sure solutions like these have been discussed before and passed over. So I'm unclear what additional considerations or criteria need to be reviewed to make this decision. It feels like more of a fundamental question about the direction of Coffeescript and how to best support the migrating the existing user base. As I said earlier the original symptom that triggered the conversation is much more trivial than the stance.

It's either:

1. Remove to conform with ES6 syntax/semantics
2. Redefine under a limited scope so the output is simple ES6
3. Support as close to CS1 as possible using more involved constructs

2 - 3 is a sliding scale. But is it one that needs to considered. Like with ES2015 Class PR mentioned there was quite of discussion around handling of @ constructor params assignment around super. I think the current implementation is fine, but I can think of ways to handle more of those use cases closer to how CS1 does. I'm just not sure it's worth entertaining the idea. This is sort of similar albeit more fundamental in the pattern it encouraged that the impact extends outside of the single method with the issue.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-23 19:27

What exactly is the method you tested in your comment? Can you show some examples?

I think the way to go from here is to first show some examples of Coffee input and desired JS output, from simplest minimal case to most complex minimal case (i.e. most complex in terms of it extends and calls super and so on and so on, but there’s no extraneous code that isn’t necessary to demonstrate the syntax).

Then we should consider whether getting to that desired JS output is something the compiler should do for you, i.e. magical behind-the-scenes hoisting etc., or if we should just write documentation for how people should refactor.

If we decide that the compiler should do it, then someone needs to step up and invest the time in writing the PR. We’re all volunteers here, and @connec has other things on his plate, so if this is important to you then you might need to invest some time. The earlier classes PR and especially https://github.com/jashkenas/coffeescript/pull/4530 should tell you exactly where to look for what needs changing.

[coffeescriptbot]

From @helixbass on 2017-05-23 20:26

I'm not clear from reading this discussion whether it's been agreed that the current situation (ie jashkenas/coffeescript#4530 having been merged) needs to change before 2 is released, so most of what follows is how I feel about that current situation (vs, as a starting point, leaving bound methods as-is):

This is a shockingly breaking change (especially as I've been learning React and admiring how much cleaner its passing-around-bound-method-callbacks style is using Coffeescript bound methods). I'm agreeing with everything @ryansolid is saying:

That this is a hugely breaking change for the (lots and lots) of people who use this style, whether b/c they use callback-passing frameworks or they just like the set-and-forget aspect of always using bound methods

That it seems to have been pushed through due to @connec's (admitted) pre-existing prejudice against bound methods

That the use case in which bound methods could actually break existing code is pretty small (@GeoffreyBooth based on some of your comments I'm not sure you understand in what specific cases something breaks -- somebody correct me if I'm wrong but I believe it's only if a bound method gets passed as a callback (eg register(@someMethod)) - not invoked (eg @someMethod()) - by some code called by super() that there is an issue)

That this is comparable to @ constructor arguments not being available until after super() runs. I wasn't familiar with this change but to me this seems like pretty much exactly the same "behavior has changed in 2" caveats would apply if bound methods are kept as-is, no? That as opposed to "code inside super() that relies on @ constructor arguments breaks", it's just "code inside super() that relies on (again, if my understanding is correct not calling but just) passing bound methods as callbacks breaks"?

The context that to me seems obvious (but I'm not sure if/where it's been discussed) is that you want to minimize breaking changes in 2. That having major breaking changes like this can/will induce Python-3-style refusing to upgrade and/or bailing from the language (which already seems to be a hot topic, not wanting to give people excuses to leave Coffeescript). So for a major breaking change like this that can arguably be considered unnecessary to get pushed through seems like a really bad idea

Not clear on how the early-stage public class properties proposal relates to this, once it's deemed a legitimate compilation target then does it provide a safe way to implement bound methods (is this what @connec is suggesting above)? In which case wouldn't it make sense to keep them (as-is or via one of the proposed compilations, with the small use case where code breaks when upgrading to 2) for now and then compile them to public class properties when that's ready? This makes me wonder if Babel supports public class properties syntax and if so how do they "safely" compile it?

So the above may be unnecessary argument if it's already been accepted that the current remove-bound-methods behavior as merged via jashkenas/coffeescript#4530 needs to change, otherwise I feel strongly that it should

So then as far as the discussion of keeping bound methods but trying to compile them in a safer/more-obviously-breaking way, I don't think I'm in as strong of a position to evaluate the proposed compilations as you guys are but the fact that @ryansolid's supports calling bound methods from within super() (just not passing them (or technically doing anything that leads to them being called with an unusual this), as distinguished above) seems like a significant plus. I could take a crack at implementing @ryansolid's proposal (with the caveat that my understanding of the backend of the Coffeescript compiler is rather superficial)?

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-23 22:33

To reiterate: bound methods were removed because we couldn’t find a way to output them within an ES class that didn’t introduce bugs or subtle issues. There were not removed because of some prejudice against bound methods. If you can propose a new output for bound methods that works in an ES class and avoids the issues that led to us removing the feature, by all means propose it and it would be received very favorably.

What is needed now isn’t more imploring of how desperately you want this feature. Please write some examples like I suggested in my comment, so we can start scoping out what bound methods support in CS2 might look like.

[coffeescriptbot]

From @helixbass on 2017-05-24 00:01

@GeoffreyBooth I get that a "let's solve it" attitude is more useful than a "let's complain about it" one. But given that there may not be a suitable solution, I think it's very important to make the case (as @ryansolid has imo also done) that you guys failed to weigh the downside of introducing a massive breaking change (by merging 4530) vs introducing a class of subtle bugs. You seem to be saying that that feature must be removed b/c it introduces these bugs. To me that's highly questionable thinking, I think (as a starting point, independent from considering ways to remove/alleviate those bugs) the extent/nature of these bugs should be weighed against the downsides of removing bound methods. If you're willing to acknowledge that such a weighing should even be done, I'm arguing that you guys (yes, led by someone who's wanted to get rid of bound methods for a long time) severely underweighed requiring so much existing code to be rewritten (it feels a bit like @ryansolid and I represent the "typical user" perspective and it's a shame that you're dismissing my "imploring" on their behalf). And again, from what I can tell there's a similar class of bugs being introduced in 2 for @ constructor args yet there doesn't seem to be this same presupposition that "we absolutely cannot introduce such a type of bug"

So let's talk solutions. If by "avoids the issues that led to us removing the feature" you mean "existing code can't break at runtime in any way"/"bound methods must always be able to be called in their proper bound form" then it doesn't seem as though anyone's seeing a way to achieve this while keeping bound methods. @connec and @ryansolid's proposed compilations are both ways of making the bugs non-subtle (by explicitly erroring at runtime rather than subtly running against an unintended this), if I understand correctly. So if that's acceptable, great. I'm not sure how much help I'd be writing examples but I'll try and implement @ryansolid's proposed compilation, I get that an implementation will contribute more to this discussion than arguing

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-24 00:31

There are no bound methods in the CoffeeScript compiler codebase, so removing them had zero effect on that code; and that’s the only common codebase we can all refer to when judging how prevalent a certain feature is. I ran a few old projects through the CS2 compiler and had to change several uses of super but I had no bound methods. For all I knew, bound methods were an obscure feature that no one used. Obviously that’s not the case, but when the only datasets we have to go on don’t use a feature at all, there’s no way for us to know whether or if a feature is popular. But that’s why we have a beta period, to get feedback like this.

It’s not clear to me what your desired solution is. Perhaps you should start a new thread with a specific proposal that has examples of “this CoffeeScript becomes this JavaScript” with all the relevant cases. Just scrolling up this thread it’s not clear to me what you would prefer bound methods be compiled into. If I were to try to take this request and run with it and make a PR, I’m not sure what I would be trying to achieve. That’s what I mean by “stop imploring”—it’s not that I’m saying that I’m deaf to your pleas, but rather that I don’t know what you’re asking for. Please get specific.

Write a comment or new issue that lays out exactly what you want bound methods to be compiled to, as if you were writing instructions for someone to go and implement your proposed output as a PR. Not that I or @connec will necessarily do so, but at least then it will be very clear what you’re going for and we can give feedback on it before you or someone else spends the time trying to implement it.

[coffeescriptbot]

From @ryansolid on 2017-05-24 06:39

@GeoffreyBooth to be fair I only setup a simple scenario of having a couple classes, one extending the other with all bound methods where each called it's super method. I threw it together in a jsfiddle in a few mins and moved around the calls a bit to validate if it is possible, but obviously far from exhaustive tested and likely not the cleanest. It just so happened the first way I tried to solve it seems work. But It isn't without tradeoffs. There is probably a better way to solve it. It was more just to keep the conversation going. So, @helixbass it needs a bit more work before we look at trying to implement it.

I'm open to spending time to work on this further. I have been just been trying to determine 2 things:

1. Is this a direction worth pursuing, since for 4530 to be merged is seemed to indicate that something directionally had changed. I realize it has been reiterated the feature was just removed for lack of solving the bug. But the solutions displayed here were both suggestions in the other thread.

2. Is the approach of generating a runtime error acceptable? It isn't too hard to capture most issues at runtime but compile time may not be possible. Is there some other technical constraint that would make certain solutions more attractive?

I will do some more work assuming both the above are true to refine my solution. I hadn't buckled down on a single solution since ultimately I just want bound class methods back. It seemed there were several ways to approach it from the threads yet 4530 was accepted which has had me concerned that it was all for not.

I do think it is important for myself and those coming in with a similar mindset understand that despite the disbelief that 4530 could ever be merged, especially over that edge case, it happened honestly. We aren't missing something here, no matter how incredulous this change seems.

[coffeescriptbot]

From @ryansolid on 2017-05-26 05:27

Ok, given where things are sitting I'm going to present what I consider currently the baseline fix, ie.. the simplest way to both bring back bound class methods and not have the bug occur silently. I realized while what I posted earlier might have some marginal benefits the solution could be reduced down to something much simpler to atleast talk about this class of solution. Procedurely it is taking the output pre 4530 and adding a line of code at the beginning of every bound method to check that the method is in fact bound as expected. Something like:

 if(this === undefined || !(this instanceof Component))
    throw new Error('Bound class method accessed without being bound by super');

This means bound methods can never be bound to a non-inherited context but that seems like a reasonable limitation.

So Coffeescript:

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

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

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

To JS

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

Component = class Component extends Base {
  constructor () {
    super(...arguments)
    this.onClick = this.onClick.bind(this)
  }

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

  onClick () {
    if(this === undefined || !(this instanceof Component))
    throw new Error('Bound class method accessed without being bound by super');
    console.log('Clicked', this)
  }
}

Or the more idiomatic example:

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

    baseMethod: =>
        # do some stuff with @

class Component extends Base
    baseMethod: =>
        super(arguments...)
        @element.addEventListener('click', @onClick)

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

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

  baseMethod () {
     if(this === undefined || !(this instanceof Base))
    throw new Error('Bound class method accessed without being bound by super');
  }
}

Component = class Component extends Base {
  constructor () {
    super(...arguments)
    this.baseMethod = this.baseMethod.bind(this)
    this.onClick = onClick.bind(this)
  }

  baseMethod () {
     if(this === undefined || !(this instanceof Component))
    throw new Error('Bound class method accessed without being bound by super');
    super.baseMethod(...arguments)
    this.element.addEventListener('click', this.onClick)
  }

  onClick () {
    if(this === undefined || !(this instanceof Component))
    throw new Error('Bound class method accessed without being bound by super');
    console.log('Clicked!', this)
  }
}

I guess the question is would this sort of approach be acceptable of throwing run time errors? I'm not sure what other types of examples to show since it's always the same code. Essentially if not being bound is the problem we check for that but otherwise leave it the way it was. So we don't impose any additional constraints outside of the can't be bound to other contexts. However, bound methods can fully inherit and be inherited and called as normal by base classes including in their constructors.

[coffeescriptbot]

From @connec on 2017-05-26 11:04

I like that approach. It's simpler than my proposal but has the same outcome. The only thing I'd suggest is that the conditional throw should be wrapped in a helper similar to babel's _classCallCheck, e.g.

function _boundMethodCheck(instance, Constructor) {
  // note that `=== undefined` is redundant, since `undefined instanceof` is safe
  if (!(instance instanceof Constructor)) {
    throw new Error('Bound instance method accessed before binding')
  }
}

...

onClick () {
  _boundMethodCheck(this, Component)
  ...
}

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-26 15:55

I know we generally try to avoid runtime checks and runtime errors, but I don't know how absolute that rule is. @lydell, do you care to comment? I assume that a runtime check, especially a tiny one such as is proposed here, is preferable to removing this feature or letting it live in a dangerous (unchecked) form?

Assuming I'm correct on that, who wants to try to implement this as a PR?

[coffeescriptbot]

From @lydell on 2017-05-26 16:04

I haven't been following along here, because bound class methods don't interest me very much and the comments have been so long. But it sounds like you've reached consensus on a way to implement them, so I'd say go for it! I don't mind the little _boundMethodCheck helper at all.

[coffeescriptbot]

From @mrmowgli on 2017-05-26 16:14

+1

[coffeescriptbot]

From @ryansolid on 2017-05-27 03:33

@connec that looks good. I also realize the limitation I was talking about in my post was imagined since a function can only be bound once anyway. So looks good all around.

@GeoffreyBooth thanks for your patience.

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-27 03:38

Happy to help. @ryansolid, do you want to take a crack at this? Unless @connec you feel inspired?

[coffeescriptbot]

From @rattrayalex on 2017-05-27 04:10

Having a stacktrace point directly at the method might be helpful to developers (which is to say, inclining the check could be nicer). Either way looks good though.

I'm a major +1 to the feature overall fwiw.

[coffeescriptbot]

From @helixbass on 2017-05-27 13:34

@GeoffreyBooth I'll try and implement this unless @ryansolid or @connec prefer to

[coffeescriptbot]

From @GeoffreyBooth on 2017-05-27 15:38

Thanks @helixbass. I think we want the helper. It's more DRY and in keeping with our style. It just means that the offending method is in the second line of the stack trace instead of the first.

[coffeescriptbot]

From @GeoffreyBooth on 2017-06-06 15:20

Is this in progress? We'll be releasing a new beta soon with the JSX PR, it would be nice to include a fix for this.

[coffeescriptbot]

From @helixbass on 2017-06-06 20:21

@GeoffreyBooth I have this working on my iss84_discuss_bound_method_runtime_check branch. Test-wise so far I've just restored the old bound method tests, I'll add some new ones and open a pull request against 2

One thing I ran into was how to handle anonymous classes (since the helper expects a class name/reference as one of its arguments) - the options I could see were (1) disallow bound methods in anonymous classes (2) forgo the runtime check for anonymous classes or (3) give anonymous classes with bound methods a name. (3) might make the most sense but I wasn't totally clear on what was going on with eg ExecutableClassBody/defaultClassVariableName (in terms of how to trigger naming of anonymous classes with bound methods). So for now I've implemented (2), as (1) seems like a potentially breaking change since anonymous classes with bound methods appear to be allowed in 1.x

[coffeescriptbot]

From @GeoffreyBooth on 2017-06-06 21:11

Forgive my ignorance if this is a stupid question, but:

• Doesn’t this issue only occur for a child class that extends a parent class?
• In your helper check, instance instanceof Constructor, instance is the child class and corresponds to this (?) and therefore can be anonymous (?) and Constructor is the parent class?
• Is it possible to extend an anonymous class?

[coffeescriptbot]

From @helixbass on 2017-06-06 21:34

@GeoffreyBooth from my understanding, yes, the edge cases can only happen when a parent class's constructor() calls a child class's bound method in a non-this-preserving way (eg as a callback)

In my implementation I followed what @ryansolid and @connec outlined above, which is that every bound method (of a base or derived class) performs the runtime check, passing its own class (not the parent class) as the second arg (ie Constructor) - the only exception currently being that bound methods in anonymous classes don't perform the runtime check b/c they don't know what to pass for Constructor

However if our understanding is correct and this issue only can happen for child class bound methods, then your suggestion makes sense and we could (1) pass the parent class as the Constructor arg (which I think you're right should always be available) as well as (2) only add the runtime check to bound methods of child classes

To be thorough/pedantic, there's already the edge case (within the edge case, ie only in a parent class constructor) where a bound method could be called with the wrong this if it's explicitly called with another instance of our child class (ie using .call()/.apply()), and this would widen that edge case (b/c it could be called with any instance of the base class, not just the child class). However I don't think anyone would say this is what we're trying to guard against, it's the case where the bound method gets passed around as a callback (and gets called without explicit this) that's relevant

[coffeescriptbot]

From @GeoffreyBooth on 2017-06-06 21:45

I’ll be honest I don’t quite follow everything you wrote, but my point was just that if it’s not possible to extend an anonymous class, and this runtime check is only necessary for extended classes, then we don’t need to worry about anonymous parent classes.

[coffeescriptbot]

From @helixbass on 2017-06-06 21:59

@GeoffreyBooth basically I think you're right that b/c it only applies in child classes we can just always pass the parent class to the check (so yup I think your ​suggestion resolves the anonymous class question nicely by making it irrelevant). I'll optimistically update the PR to behave this way but please @ryansolid @connec or whoever chime in if you think it should remain as specified ie check against its own class and check for all bound methods not just child class bound methods

[coffeescriptbot]

From @ryansolid on 2017-06-07 03:53

Right so classes that aren't extended don't need the check. That makes sense since there is no issues around the timing of calling super.

And since a child of a child is still an instance of the Base I don't see any issues. If the base class bound method is overridden by a bound or unbound version that base class will still call bind on the method before any chance of the child method being called. And since the method can only be bound once that checks out.

[coffeescriptbot]

From @GeoffreyBooth on 2017-06-30 17:41

Hopefully fixed via https://github.com/jashkenas/coffeescript/pull/4594