Skip to content
This repository has been archived by the owner on Feb 19, 2018. It is now read-only.

CS2 Discussion: Output: Classes #22

Closed
rattrayalex opened this issue Aug 6, 2016 · 91 comments
Closed

CS2 Discussion: Output: Classes #22

rattrayalex opened this issue Aug 6, 2016 · 91 comments

Comments

@rattrayalex
Copy link
Contributor

This is a core feature that we should rally around submitting to jashhkenas/coffeescript in the near term.

Does anyone know of current leading efforts to accomplish this in the community? Perhaps we can lend a hand there.

@kirly-af
Copy link
Contributor

kirly-af commented Aug 7, 2016

This is discussed in jashkenas/coffeescript#4233 (and probably many others I'm not aware of).

Also, I just discovered this Redux PR thanks to @GeoffreyBooth which looks REALLY promising.

@GeoffreyBooth
Copy link
Collaborator

So it appears that someone has actually already implemented CoffeeScript class to ES2015 class—in decaffeinate. Check it out!

@DomVinyard
Copy link

DomVinyard commented Aug 15, 2016

I think there's probably a larger conversation to be had about decaffeinate specifically, how and where it intersects with this project, what can be borrowed, what can be adapted, etc.

@alangpierce
Copy link

Hi, I'm a decaffeinate contributor. While decaffeinate does have basic support for classes, full support is one of the main missing features right now. I'm hoping to improve it over the next few weeks, but it's definitely a challenge, since my understanding is CoffeeScript allows arbitrary code in class bodies, while ES2015 classes only allow methods. Decaffeinate also compiles CoffeeScript class fields to the public class fields proposed feature (currently stage 2), but even that has pretty significant semantic differences that have caused bugs for me (CS class fields are on the prototype, JS class fields are assigned to the instance once the constructor finishes). Anything other than methods or fields causes decaffeinate to crash.

@GeoffreyBooth
Copy link
Collaborator

Thanks @alangpierce. Yeah, after looking at it more I think the better solution is to redefine the class syntax in CoffeeScript to be something that translates directly into ES2015 classes, dropping support for ES2015-unsupported features like code in class bodies. This would be a breaking change in CoffeeScript, probably one that’s opt-in via a flag, but necessary if CoffeeScript is going to keep up with new features introduced by ECMAScript.

While the immediate focus is on adding support to CoffeeScript for missing vital ES2015 features (modules, classes), the longer-term goal especially of coffeescript6 is to update CoffeeScript’s output to use more of the latest standards. This essentially means we would be doing the work that decaffeinate is doing, but within the main CoffeeScript compiler. I skimmed through the decaffeinate repo, hoping to find some commits that highlight what differences it had from the main CoffeeScript compiler, but I couldn’t make sense of it. Do you mind giving whatever overview you can of how decaffeinate works, and where I should look for code that might be useful to repurpose in the main CoffeeScript compiler? For example, where is the code that generates an ES2015 class?

@mrmowgli
Copy link

I would love to see some way of extending the current class functionality to generate the ES2015 code, and then decorate with everything else. I guess I don't really understand what actually breaks and where yet though. I am assuming that ES classes have extra restrictions, ie that they aren't just objects with prototypes set in a specific pattern?

From the Babel overview:

ES2015 classes are a simple sugar over the prototype-based OO pattern. Having a single convenient declarative form makes class patterns easier to use, and encourages interoperability. Classes support prototype-based inheritance, super calls, instance and static methods and constructors.

So I would think we could construct functions only, then decorate with everything else?

@alangpierce
Copy link

@GeoffreyBooth Yep, dropping support for code in class bodies makes a lot of sense for your case.

Sure, I can give a quick overview of how decaffeinate works:

Unfortunately, I think the short answer is that decaffeinate's approach is pretty different from the CoffeeScript compiler, so you may not be able to use much code/logic. From my reading of the CoffeeScript compiler, it parses the input to an AST, then calls the recursive compileNode method (and related methods) to generate a big JavaScript string that becomes the output file. Decaffeinate works by parsing the input into an AST (and a token sequence), then using that AST to inform targeted transformations on the code (inserts, removes, and replacements) using the magic-string library. For example, to compile a b, it uses the AST to see that it's a function call, looks at the tokens to see that there's no open-paren or close-paren, and replaces the space with a ( and inserts a ) after the b (code here). This approach makes sense given decaffeinate's goal: it doesn't want to rewrite all of your code, it wants to tweak it (sometimes significantly) to be valid JavaScript, maintain formatting and comments as much as possible. But really, this approach ends up being a big pain sometimes, and transforming the code to a JavaScript AST would probably be more robust in a lot of ways (but would lose formatting).

A little more information on how decaffeinate is structured: it consists of five "stages" (one CoffeeScript -> CoffeeScript, one CoffeeScript -> JavaScript, and three JavaScript -> JavaScript), the most important of which is the CoffeeScript -> JavaScript stage, which is called MainStage. For that stage, every type of AST node corresponds to a Patcher class. Each Patcher is a class that knows what kind of modifications to make to transform that type of CoffeeScript node to JavaScript.

Here's a repl link that has some examples of what happens in different stages. You can change the "run to stage" selection to see the intermediate result after each stage (or intermediate lexer/parser results).

I actually haven't worked with the class generation code, but was able to mostly figure out how things work. If you look at the decaffeinate-parser output for your example, you can see that the main AST hierarchy is Class > Block > ClassProtoAssignOp > Function. From the node name to patcher class mapping, (or by just looking at the names), you can see that they map to the classes ClassPatcher, BlockPatcher, ClassAssignOpPatcher, and FunctionPatcher. The methods patchAsStatement and patchAsExpression are generally the most important ones. As one example, ClassAssignOpPatcher extends ObjectBodyMemberPatcher, which calls this.expression.patch with the method flag set appropriately. On the FunctionPatcher side, it knows that it should skip the function keyword if the function is being patched as a method.

Also, here's a writeup with some more details:
https://github.com/decaffeinate/decaffeinate/blob/master/docs/contribution-guide.md

Hope that helps!

@GeoffreyBooth
Copy link
Collaborator

I don’t think we can simply redefine the class keyword, even behind a flag. That would be too drastic a breaking change for current projects to handle, including the CoffeeScript compiler codebase itself (which uses class extensively). I think we need to provide a clear and easy upgrade path, especially if we’re adding ESNext class support to the original compiler.

How about this approach: we create a new CoffeeScript keyword, esclass, that has different syntax than the current class. It only allows things that can be output into an ESNext class, and it compiles into ES output (so class is in the generated JavaScript, not function).

With esclass, a CoffeeScript project could contain both new compile-to-ESNext-class classes and legacy old compile-to-function classes. People could convert a project gradually, or pick and choose which type of classes they want to use (for example, esclass could be used when extending a class imported from another library, with class used for all classes particular to your project). Having two types of classes would also let us keep the CoffeeScript compiler codebase intact without major changes.

Introducing a new keyword that wasn’t previously reserved is a breaking change, so we would need to introduce a flag. I think since esclass is so obscure a string, it’s unlikely that there are many (if any) projects out there using it already as a symbol; so maybe we could get away with shipping it reserved by default, so that the vast majority of projects need not do anything to take advantage. For the 0.001% of projects using esclass already as a variable or function name (and who don’t want to refactor), they would need to set the flag to force legacy support. The flag could work something like this:

  • Default, no flag: The class keyword would retain its current syntax, and esclass would become a new reserved word that introduces a new syntax that compiles into ESNext classes.
  • --legacy-classes: The current behavior. class is unchanged, and esclass is not a reserved word.

We could even add support for making class an alias for esclass and csclass an alias for the current class, if we want to let people use the class keyword for the new syntax; but I’m concerned that it’s probably a bad idea to not be able to look at code and know how it will get compiled, if you don’t know what flag will be set in the compiler.

Conversely, it might be a good idea to introduce csclass as a new reserved word that always aliases to the current class syntax, so that people could use esclass and csclass exclusively, leaving no ambiguity about what their intentions are.

What do you all think? @lydell, @rattrayalex, @JimPanic, others . . .

@JimPanic
Copy link
Contributor

JimPanic commented Sep 6, 2016

I like the idea of csclass & esclass although we might want to find better names. I also am not sure yet what the differences in semantics are. It seems there is a bit more to it than just syntactic sugar as the ES201x class does not allow data properties, has different grammatical rules than object literals and cannot be called without new (if I read that correctly).

Apart from that, I think the only solution to introduce this to users is a) with conversion tools and b) through a process of deprecation warnings, deprecation and removal over several releases. This is something we have to plan, no matter what keywords we use in the end.

The only thing that could be dangerous is to keep the class keyword to stay semantically different from the one in ES201x. Its meaning has to change eventually.

@lydell
Copy link

lydell commented Sep 6, 2016

Here's my vote:

Add esclass for ES2015+ classes. Make no other changes whatsoever.

Release it by bumping the middle version number, keeping CoffeeScript's current versioning scheme of bumping the middle number when there are new features an slightly-but-not-very breaking changes. (As @GeoffreyBooth it is very unlikely that this will cause any trouble for anyone.)

No csclass. Do we want csin, csof, csfor etc. too?

CoffeeScript 2.0.0: Remove old class, rename esclass into class (and do similar things for other ES2015+ features) and switch to semver.

@GeoffreyBooth
Copy link
Collaborator

I guess we probably don’t need a flag if we’re simply adding esclass (or whatever people decide to name it) and leaving class alone. It would be a breaking change to anyone who was already using esclass as a symbol, but maybe rather than giving them a flag to opt out we should just recommend they simply don’t upgrade (or that they refactor). It’s a very minor refactor to find and replace all instances of esclass with something else, we wouldn’t need to provide migration tools for that.

My god, did we just come up with a way to implement classes without a flag? 😛

@JimPanic
Copy link
Contributor

JimPanic commented Sep 6, 2016

I think we did, yes. 🤔 I'm also very much in favour of implementing semver.

@greghuc
Copy link

greghuc commented Sep 6, 2016

I like the idea of a separate esclass as a 'least breakage' way forward. However, I'm less comfortable with how the eventual replacement of Coffeescript's class with esclass could pan out.

At present class let's me do some nice things: instance functions, static functions, private static functions and 'constants'. I've written a short example class below. If esclass doesn't eventually support these class capabilities, then this is a step backwards for Coffeescript. Though you could argue that if ES2015+ classes can't handle this, then that's a step backwards too!

Example:

  class ClassExample

    CONSTANT_EXAMPLE = 'I am a constant example'

    constructor: -> console.log 'I am a constructor'

    instanceFunction: ->
      console.log '[start] I am an instance function'
      console.log CONSTANT_EXAMPLE
      privateStaticFunction()
      @constructor.staticFunction()
      console.log '[end] I am an instance function'

    @staticFunction: ->
      console.log '[start] I am a static function'
      console.log CONSTANT_EXAMPLE
      privateStaticFunction()
      console.log '[end] I am a static function'

    privateStaticFunction = -> console.log 'I am a private static function'

  # Example calls
  #ClassExample.staticFunction()
  (new ClassExample).instanceFunction()

@lydell
Copy link

lydell commented Sep 6, 2016

@greghuc I don’t really see the step backwards?

# ClassExample.coffee

CONSTANT_EXAMPLE = 'I am a constant example'

privateStaticFunction = -> console.log 'I am a private static function'

export class ClassExample

  constructor: -> console.log 'I am a constructor'

  instanceFunction: ->
    console.log '[start] I am an instance function'
    console.log CONSTANT_EXAMPLE
    privateStaticFunction()
    @constructor.staticFunction()
    console.log '[end] I am an instance function'

  @staticFunction: ->
    console.log '[start] I am a static function'
    console.log CONSTANT_EXAMPLE
    privateStaticFunction()
    console.log '[end] I am a static function'

# whatever.coffee
import {ClassExample} from 'ClassExample.coffee'
ClassExample.staticFunction()
(new ClassExample).instanceFunction()

@greghuc
Copy link

greghuc commented Sep 6, 2016

@lydell I'm still getting up to speed with ES6 features, so please take my comments with a pinch of salt..

Your example code isn't that bad. It is essentially forcing a coffeescript user to have a closure wrapping both the class declaration and the private 'constant' and static function, to ensure private means private. But from my quick scan of ES6 modules, I suppose there's an implicit closure when importing a module from a file.

I could live with this, although I don't think your example code is as clean or explanatory as the current coffeescript code. In the current code, the class acts as an encapsulating namespace for all private and public functions.

For the esclass to class transition, there'll be a need to document how 'private' variables and functions are implemented in the new world.

@DomVinyard
Copy link

How about, instead of esclass we go for Class, uppercase C?

@lydell
Copy link

lydell commented Sep 6, 2016

@DomVinyard In my opinion that would be very confusing. I also think that that will break more people's code – even CoffeeScript's source code has class Class in it!

@DomVinyard
Copy link

@lydell I concede that point and do accept it would break more projects than esclass would (although still not a great deal in the grand scheme of things). It's just that Coffeescript seems too institutionally elegant and natural for keyword prefixes. Your illustration earlier of "why not esfor or esin?" describes the danger better than I could.

@GeoffreyBooth
Copy link
Collaborator

@DomVinyard I have to agree with @lydell on this one. I’m not thrilled by the name esclass, but I think Class could be way too confusing. It’s easy to rename later if someone proposes a better name.

So I think the next step is to define what the syntax of esclass would be. I don’t really follow the example above. The relevant pages on MDN are here and here and here. Some interesting things:

  • Class declarations are not hoisted (unlike function declarations).
  • When used in a constructor, the super keyword appears alone and must be used before the this keyword can be used. This keyword can also be used to call functions on a parent object.

Here’s an extensive example.

It seems clear from the examples that we will need to implement get and set as part of this, and some way to trigger output of the static keyword. super will also behave differently when inside an esclass—it would be called like super.parentClassMethodName rather than just super, except in the constructor.

I think like with modules, our goal should be to simply create a way to output ES classes with all their features as they are, rather than trying to improve upon them the way that the original class improved upon “function classes.” At least at first; we could add sugar later.

Is anyone else interested in helping research/define this, and/or code it?

@rattrayalex
Copy link
Contributor Author

Exciting stuff.

Perhaps once esclass is implemented we'll even find that it can replace class. Either way, makes sense to me to take that tack.

I am curious though – what advantages do CS classes have over ES classes? Why would a developer prefer CS to ES output? (Honestly don't know).

@JimPanic
Copy link
Contributor

JimPanic commented Sep 8, 2016

@rattrayalex ES201x classes cannot have executable code in their bodies. You cannot declare properties at runtime. You can do that with CS classes but I personally think it's bad practice to do so as it's not very explicit and mixes different styles of defining "types".

@JimPanic
Copy link
Contributor

JimPanic commented Sep 8, 2016

Ok let me rephrase that a bit: of course you can declare properties at runtime, but not in the class declaration body like in CS. :)

@greghuc
Copy link

greghuc commented Sep 8, 2016

Re-reading through this 'classes' thread, I think it's worth noting that are at least three distinctive positions to take on Coffeescript and ES6 classes. Two positions are at odds with one another. The positions:

  1. Coffeescript classes are semantically fine as they are. The current Coffeescript compiler implements classes using correct use of Javascript prototypes. This enables instance functions, static functions, private static functions and 'constants' - fantastic! In the meantime, ES6 has formalised one approach to implementing Javascript classes. Quoting from @mrmowgli's comment above and MDN: "JavaScript classes are introduced in ECMAScript 6 are syntactical sugar over JavaScript's existing prototype-based inheritance. The class syntax is not introducing a new object-oriented inheritance model to JavaScript". From this perspective, we should update the CS compiler to output CS classes as ES6 classes, and decorate them afterwards to provide class features that CS currently has and ES6 has not. @lydell's comment above shows how this could be done.
  2. Coffeescript classes are semantically wrong, now that ES6 classes have come along. From this perspective, we should reimplement ES6 classes, but in a Coffeescript syntax. We should remove all CS class features that ES6 currently doesn't support. We need to resolve backwards compatibility: we either define CS class behaviour as ES6 behaviour and accept existing code will break, or we postpone the issue by introducing a new esclass.
  3. Coffeescript classes and ES6 classes are technically incompatible. @GeoffreyBooth mentions that Coffeescript should output to ES6 classes in order to "interoperate with libraries that use ECMAScript classes". I haven't seen a concrete example of this yet - it would be great to have some broken test-cases showing this behaviour. If Coffeescript and ES6 are becoming technical inoperable, then this definitely needs fixing.

My personal position is a combination of 1 and 3: change the Coffeescript compiler to take advantage of ES6 class functionality, and fix any technical incompatibilities. But don't change (or remove!) existing CS class semantics.

I see position 2 as a risky one (Coffeescript classes are semantically wrong, now that ES6 classes have come along). This path leads to the ES6 tail wagging the Coffeescript dog. From now on, Coffeescript is no longer an opinionated "little language that compiles into JavaScript". Instead, we'll be continually playing catchup, discussing the best way to implement the newest EcmaScript feature in Coffeescript syntax. In essence, it will be Ecmascript committees driving the core direction of Coffeescript, not the Coffeescript committers themselves. Already, because ES6 classes don't currently support data properties, we might have to remove Coffeescript class data properties. What happens when Ecmascript adds them back in?

@lydell
Copy link

lydell commented Sep 8, 2016

@greghuc The biggest problems, in my opinion, are that you currently cannot extend an ES2015+ class (jashkenas/coffeescript#4233) and that CoffeeScript’s super is both very complicated and very different from super in ES2015+ (jashkenas/coffeescript#3796 might be informative).

@greghuc
Copy link

greghuc commented Sep 8, 2016

Also, to put a slight dampener on 'ES6 classes are the new awesome', here's a link to a curated list of resources on why ES6 (aka ES2015) classes are NOT awesome.

I'm not aiming to start an argument over the pros and cons of ES6 classes. However, it's worth keeping in mind that ES6 classes are a new and unproven technology, and it's unclear how things will play out with them. This is independent to how well ES6 classes are supported by browsers and nodejs.

This raises the possibility of Coffeescript nextgen locking itself to a feature that doesn't stand the test of time.

It doesn't preclude Coffeescript being able to interoperate with ES6 classes.

@GeoffreyBooth
Copy link
Collaborator

@greghuc I think your preferred approach (1 and 3) is what the esclass proposal does. We leave the current class alone, in all its awesomeness, and introduce a new esclass that matches ECMAScript’s class definition. If ECMAScript adds data properties, we add data properties to esclass. Yes, this means we are following the ECMA standards body. But . . . CoffeeScript is a language that compiles to JavaScript. By definition the ECMA committee tail will wag the CoffeeScript dog.

What your “data properties” example illustrates, however, is that we should think long and hard before we add any sugar over esclass. Since ECMAScript’s class is subject to change, we very well could get ourselves into trouble if we add custom features on top of it. In general now that JavaScript is no longer static, we should refrain from implementing any new custom features that might someday get implemented natively in ECMAScript (especially if they might get implemented in an incompatible way, like class or ...). Presumably ECMAScript won’t break backward compatibility with the current spec for class, so we can at least implement that; but no more than that, until ECMA finalizes those new additions.

I don’t think it’s so bad to follow ECMA. They’re moving way faster than we ever could. It’s not a bad thing to keep up with the JavaScript world.

@GeoffreyBooth
Copy link
Collaborator

I’ve been thinking about how Decaf manages to parse most CoffeeScript classes into ES2015 classes. If we’re going to introduce ESNext output via a flag or 2.0.0 breaking-change version bump, then we could take Decaf’s approach and output ES2015 class whenever the CoffeeScript class isn’t doing anything incompatible (like code in class bodies) and the current function otherwise. What would people think of this approach?

Out of curiosity I ran Decaf on nodes.coffee. It throw an error on any statement like [..., last] = body.expressions, but if I commented them out, the only class it couldn’t parse was Op, because of the lines starting with CONVERSIONS = and INVERSIONS = that technically are code in a class body. But I feel like those could probably be refactored as @CONVERSIONS = and just placed inside the constructor. Ditto for the children: lines that Decaf simply loses in its conversion.

It’s another question whether these ES2015 classes are truly functionally equivalent to the CS-generated function versions. To figure that out, we’d have to either refactor all the [..., last] lines throughout the code or fix Decaf to handle such lines; and refactor any other classes with class bodies; and update the test runner to use a version of CoffeeScript that was run through Decaf and then Babel. No small task, but perhaps worth it if we learn that most uses of class could be output as ES2015 as is. Here is Decaf’s output for my modified nodes.coffee (that comments out the troublesome lines); aside from the missing children: properties, is there anything here in its conversion of classes that stands out to anyone as being problematic?

Assuming we wouldn’t use Decaf but would reimplement similar logic ourselves, is this an approach we want to take?

@connec
Copy link

connec commented Dec 16, 2016

@mrmowgli regarding super:

Firstly, the way super is compiled in CS, roughly, is that a "super reference" is determined based on the containing method's name and context, some current examples:

Context.name = -> super             # doesn't compile
Context.prototype.name = -> super   # super reference: Context.__super__.name

class Context then @name = -> super # super reference: Context.__super__.constructor.name
class Context then name: -> super   # super reference: Context.__super__.name

# constructor is not special
Context.prototype.constructor = -> super   # super reference: Context.__super__.constructor
class Context then constructor: -> super   # super reference: Context.__super__.constructor

In jashkenas/coffeescript#4354 (the active ES class PR) the only change for super is the last example above, all others have been left alone.

class Context then constructor: -> super # super reference: super

Ordinarily, the "super reference" is compiled with a .apply(this, arguments) or a .call(this, arg1, arg2, ...) depending on how super is called (bare super vs. super(arg1, arg2), etc). This doesn't work in an ES constructor because super in a constructor seems to be a very restricted syntactic form, rather than an 'auto' reference like arguments, hence when compiling a constructor super call there's a special case when the super reference is simply "super".

this doesn't exist in an extended class until super() is called.

That is an interesting case indeed. The way I chose to handle it in my PR is to add an implicit super to all derived constructors unless there's already an explicit one, e.g.:

class A
class B extends A then constructor: ->
# ...
#   class B extends superClass {
#     constructor() {
#       super(...arguments);
#     }
#   }
# ...

class A
class B extends A then constructor: -> super()
# ...
#   class B extends superClass {
#     constructor() {
#       super();
#     }
#   }
# ...

The reason I chose to do this is because it's the most reasonable way I could think of to deal with @-params and bound methods. Consider:

class B extends A
  constructor: (@name) ->

  greet: ->
    console.log "Hi #{@name}"

class C extends A
  constructor: (name) ->
    super()
    @name = name

  greet: =>
    console.log "Hi #{@name}"

Without an implicit super, class B is clearly broken: it doesn't call super so attempting to access its internal state will always fail. However, we can't simply add an explicit super as CS will compile the @-param into an assignment at the top of the constructor:

class B extends A {
  constructor (name) {
    this.name = name
    // super() - this is already too late
  }
}

C is also broken, though slightly more subtly: although it doesn't have @-params or reference this before calling super, it does make use of bound methods (greet: => ...). CS compiles method bindings into calls at the top of the constructor:

class C extends B {
  constructor (name) {
    this.greet = bind(this.greet, this)
    super() // - this is also too late
  }
}

Given these cases, there are basically 3 options:

  1. Disallow @-params in constructors and disable bound instance methods in extended classes (serious blow to backwards compatibility and utility, imo).
  2. Insert @-param assignments and method binding calls after the first super in a derived constructor. I haven't really explored this option, but I'm concerned that performing the insertion would be non-trivial and error-prone if super was used as part of an expression (e.g. result = super() is a valid expression, as is result = [super(), this.name]). I expect it could be done but have no doubt it would generate pretty ugly code.
  3. Add an implicit super, at the top of the function, to constructors with @-params or bound methods, and disallow explicit super in these cases. This preserves the original compilation of @-params and bound methods and results it more predictable behaviour, imo.

I went with an extension of 3., and decided to add an implicit super in all cases, which is easier to explain and understand. If a user strongly cares about how super is called, they would need to forgo using @-params or bound methods.

@connec
Copy link

connec commented Dec 16, 2016

Sorry for the wall of text! Figured I'd try and explain the current status as fully as I could.

Having written that all out, I'm starting to feel like option 2. should be explored a little more. I guess the idea behind that would be to generate code that made it seem like @-param assignments and method bindings occurred as part of the call to super.

// class A
// class B extends A
//   constructor: (@foo) ->
//     console.log super(), @foo, @bar
//   bar: =>
class A {}
class B extends A {
  constructor (foo) {
    var ref;
    console.log((ref = super(), this.foo = foo, this.bar = this.bar.bind(this), ref), this.foo, this.bar)
  }

  bar () {}
}

This would behave reasonably well I think, as attempting to access @name or @greet before calling super would throw this is undefined, so from the user's perspective it doesn't matter that @name hasn't been assigned yet.

@mrmowgli
Copy link

mrmowgli commented Dec 17, 2016

Ok, my bad: Super CAN be called in other methods, and super isn't required except in constructors for this:

var A = class A {
  constructor (token) {
    this.token = token;
    console.log('Token in base was: ', token); 
  }  
  objectLocal(origin) {
    console.log('The '+ origin + ' called in Parent method.'); 
  }
}

var B = class B extends A {
  constructor (token){
    super(token);
    this.token = token;
    console.log('Secondly, token `'+ this.token + '` was called in child.');  
  }
  objectLocal() {
    console.log('Overriding objectLocal now with "super"'); 
    console.log('Current token: ' + this.token) 
    super.objectLocal(this.token)
  }
}

let n = new B('coffee-script');
n.objectLocal();

/* Output:
  Token in base was:  coffee-script
  Secondly, token `coffee-script` was called in child.
  Overriding objectLocal now with "super"
  Current token: coffee-script
  The coffee-script called in Parent method.
*/

@GeoffreyBooth
Copy link
Collaborator

@connec @mrmowgli, fantastic heroic work on jashkenas/coffeescript#4354. Now that that’s merged in, what work related to classes remains to be done?

@mrmowgli
Copy link

mrmowgli commented Jan 19, 2017

I realize we didn't create browser specific tests, however I believe we are expecting Babel to cover the bases there. Other than that it seems like it's mostly edge cases surrounding super(), and those need actual usage to shake out a bit. I'm of the opinion we should go ahead with the release, the tests are comprehensive and everything works far better than I originally expected. @connec did an amazing job!

Realistically I believe we should start trying to cover lower priority items like get/set, species, and new modifiers like new.target . I don't believe these should hold up a 2.0 release however.

@connec
Copy link

connec commented Jan 19, 2017

The only thing we probably want to finish off for 2 is super in regular methods. Ultimately this is optional (things will work as they are), however it would be a pretty big win for output clarity and reducing complexity in the compiler.

Unless someone else picks it up I will work on this once I make some time.

@GeoffreyBooth
Copy link
Collaborator

@mrmowgli the tests really only cover Node, aside from the browser-specific ones. We treat Node as more or less our reference JavaScript runtime to test against, for JavaScript features that should behave identically in Node and browser-based runtimes.

@connec what do you mean by super in regular methods? Can you give some examples of current output and desired output? And how soon do you think you might have more time?

@connec
Copy link

connec commented Jan 19, 2017

CS source

class Base
  method: ->
    console.log 'Base#method'

class Child extends Base
  method: ->
    super
    console.log 'Child#method'

Current CS2 output

// Generated by CoffeeScript 2.0.0-alpha
var Base, Child;

Base = class Base {
  method() {
    return console.log('Base#method');
  }

};

Child = (function(superClass) {
  class Child extends superClass {
    method() {
      Child.__super__.method.call(this, ...arguments);
      return console.log('Child#method');
    }

  };

  Child.__super__ = superClass.prototype;

  return Child;

})(Base);

Desired CS2 output

// Generated by CoffeeScript 2.0.0-alpha
var Base, Child;

Base = class Base {
  method() {
    return console.log('Base#method');
  }

};

Child = class Child extends Base {
  method() {
    super.method(...arguments);
    return console.log('Child#method');
  }

};

I could have time any day now 😄 Watch this space at the weekend...

@connec
Copy link

connec commented Jan 19, 2017

There will be some 'tough choices' to make for this, I expect. I've not explored it much yet but if we wanted to clean up properly we would effectively lose our current support for super in assignment-based classes, e.g.

Base = ->
Base::method = -> console.log 'Base#method'

Child = ->
Child extends Base

Child::method = ->
  super
  console.log 'Base#method'

Some alternatives:

  • Leave the existing system hanging around, and lose some of the compiler-cleanup wins.
  • Replace __super__ with .getPrototypeOf and continue to support super in plain functions through that. This would clear up classes, but would make plain function super slower (would need to call Object.getPrototypeOf for every super call).

I'll flesh all these out a bit once I started looking at it.

@GeoffreyBooth
Copy link
Collaborator

We already removed some tests that assumed a class was really a function (i.e. the pre-ES2015 class implementation). Your example that begins with Base = -> assumes that implementation, which I think should be considered unsupported in CS2. I think we should only allow super within methods that belong to classes, and in CS2 the only way to create a class is to use the class keyword.

When you get a chance, you should start documenting the class-related breaking changes in the wiki. You’re far more familiar with what’s changed than I am.

@jashkenas
Copy link

jashkenas commented Jan 19, 2017

ES6 super doesn't work in methods added to the class prototype from other locations? Seriously? That's insane.

Edit: Regardless — moving to ES6 classes for CS2 means that you take the lumps that come with. We should definitely not still have the original system still lying around in the codebase for the CS2 release.

@connec
Copy link

connec commented Jan 19, 2017

Nope @jashkenas, it's a 'syntactic form' that is only valid within a class initializer.

@connec
Copy link

connec commented Jan 19, 2017

I think it's interesting that we could opt to compile super into Object.getPrototypeOf(this)[method] in basically any context. Only cost is the extra call to Object.getPrototypeOf, rather than caching it somewhere (such as __super__, currently).

@connec
Copy link

connec commented Jan 19, 2017

Threw together jashkenas/coffeescript#4424 with the bare bones.

@jashkenas
Copy link

Nope @jashkenas, it's a 'syntactic form' that is only valid within a class initializer.

It's just madness. It straightjackets class construction unnecessarily, and makes ugly lots of interesting patterns for mixing methods into classes, or composing traits. One step forward, three steps back.

I think it's interesting that we could opt to compile super into Object.getPrototypeOf(this)[method] in basically any context. Only cost is the extra call to Object.getPrototypeOf, rather than caching it somewhere (such as super, currently).

That seems like a pretty nice CoffeeScript feature that would improve upon ES6 classes.

@GeoffreyBooth
Copy link
Collaborator

Closed via jashkenas/coffeescript#4424. For further changes or improvements upon CS2 classes, please open new issues.

@coffeescriptbot
Copy link
Collaborator

Migrated to jashkenas/coffeescript#4922

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests