Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending an ES6 class with a CS class fails #4233

Closed
bcherny opened this issue Mar 23, 2016 · 28 comments
Closed

Extending an ES6 class with a CS class fails #4233

bcherny opened this issue Mar 23, 2016 · 28 comments
Milestone

Comments

@bcherny
Copy link

bcherny commented Mar 23, 2016

CoffeeScript calls classes as functions (rather than newing them as constructors) when instantiating a class that inherits from an ES6 class.

The fix is to interoperate with ES6 classes - no need to go all the way and compile Coffee to ES6 classes. We should check to see if a class is an ES6 class, and treat it differently (eg. see what angular does here).

Test case:

js:

class A {}

coffee:

class B extends A {}
new B # TypeError: Class constructor A cannot be invoked without 'new'
@lydell
Copy link
Collaborator

lydell commented Mar 23, 2016

(Actually, you’re doing A({}) (that is, invoking A without new) in the first line. But your issue still stands if that accidental {} is removed.)

@bcherny
Copy link
Author

bcherny commented Jun 7, 2016

This is still a major blocker for us - any way you guys can push a patch to interoperate with classes??

@michaelficarra
Copy link
Collaborator

@bcherny Someone would have to suggest a fix first. I don't believe that there's going to be a way to do this. It could be done with Function.prototype.bind, but we can't use that in CoffeeScript's target language.

@bcherny
Copy link
Author

bcherny commented Jun 8, 2016

I see what you're saying, what a bummer. What about an opt-in compiler flag, something like --allow-bind so this kind of interop is supported?

@michaelficarra
Copy link
Collaborator

The CoffeeScript maintainers have rejected compilation target options many times in the past, but it may be accepted if that's the only way to do something.

@vendethiel
Copy link
Collaborator

I think an ES6 compilation target was almost-agreed upon?

@bcherny
Copy link
Author

bcherny commented Jun 8, 2016

What's the compile target today? It seems that CS already makes exceptions for generators. This one special case for interoperable classes would be a huge win for my team!

@carlsmith
Copy link
Contributor

I'm not certain on the details, but I think generators are different, because there, you opt in to ES6 dependance by using generators. Generators compile to ES6 code. Classes are different, as people use CoffeeScript classes in code that is expected to work in old browsers, so you'd need a flag or something.

@albertywu
Copy link

👍

@connec
Copy link
Collaborator

connec commented Jun 9, 2016

You could use the same trick angular uses to determine whether or not to use bind:

...
function B () {
  if (/^class /.test(A.__super__.constructor)) {
    // A extends an ES6 class so can't be called directly
    return new (Function.prototype.bind.apply(B.__super__.constructor, [ null ].concat(slice.call(arguments)))
  } else {
    // A extends something else (presumably a function), so apply is fine
    return B.__super__.constructor.apply(this, arguments)
  }
}

If class exists, so must bind (I assume...)

@bcherny
Copy link
Author

bcherny commented Jun 14, 2016

@connec That's a great point! Pinging @michaelficarra for comment..

@connec
Copy link
Collaborator

connec commented Jun 15, 2016

One issue with that actually is the return new which will always return an object I guess... so this wouldn't interact well with class constructors returning objects, e.g. new ... always returns an object, so function B () can't determine whether it was returned explicitly by the constructor or not.

You could deal with some cases by testing instanceof A.__super__.constructor and returning if not, but that won't help if, for some reason, the constructor specifically returns an instance of itself. The return value would end up being ignored.

class BadAncestor
  constructor: ->
    return Object.create(BadAncestor.prototype)

class Descendant extends BadAncestor

new Descendant # BadAncestor {} currently / Descendant {} if changed

@bcherny
Copy link
Author

bcherny commented Jun 15, 2016

@connec I'm not sure I understand. From your code sample, it looks like this is already an issue today. How would interop with ES6 Classes make this any different?

@connec
Copy link
Collaborator

connec commented Jun 15, 2016

Sorry, I jumped around a bit, I'll try and explain it properly.

For context, just in case, Javascript has this funky behaviour whereby using new with a constructor that returns an object will evaluate to the returned object, whereas returning any non-object will evaluate to the new instance.

function A () {
  return { hello: 'world' }
}
new A() // { hello: 'world' }

function B () {
  return 'not an object'
}
new B() // B {}

The current return B.__super__.constructor.apply(this, arguments) propagates this behaviour quite nicely to the child class. E.g., if the superclass constructor returns an object, child classes will return the same object, and if it does not, child classes will return an instance of themselves.

The proposed compilation of return new (...) break this propagation,as new (...) will always return an object: it will return an object if the superclass constructor returns an object, and it will return a new superclass instance if it does not. This would mean that a child class implemented in this way would never return an instance of itself (unless it was to override the constructor).

Initially I thought it might be possible to check if the result of calling the superclass constructor was an instance of the superclass, and if so assume the superclass constructor didn't return anything. However this doesn't work if the superclass constructor explicitly returns an instance of the superclass...

What a mess.

I'm inclined to believe the only way for this to work properly is to just emit an extends, which would mean using eval to avoid syntax errors or just targeting ES2015.

@bcherny
Copy link
Author

bcherny commented Jun 16, 2016

@connec Thanks for the explanation - I didn't understand that the broken case is a super's constructor returning something.

Coming from more strictly OO languages, it actually seems pretty problematic that this is ES6's behavior today (I guess it makes sense, since class is just sugar for function):

class A {
  constructor(){
    return {}
  }
}
class B extends A {}
new B instanceof A // false
new B instanceof B // false

Anyway, I've created runnable test cases for the 4 scenarios, which I think address all the variants:

@bcherny
Copy link
Author

bcherny commented Jun 16, 2016

Actually, what if we make this even simpler (what @carlsmith was getting at):

  • --es6-classes CLI flag emits CS classes as ES6 classes

That's it. No need for worrying about reproducing ES6 class extend semantics with functions. It's opt-in, same as generators.

@bcherny
Copy link
Author

bcherny commented Jun 20, 2016

What's the process for getting this implemented? Is there a particular contributor that needs to sign off on this?

I would put up a PR myself, but I am not confident in how to best make this change.

@anodynos
Copy link

anodynos commented Jul 7, 2016

@bcherny this would be a breaking change for 1.x, cause coffeescript classes can have inner bodies and plain properties (not methods) that ES6 class can't have.

The way forwards is to go for Coffeescript version 6, see my #4078 (comment)

@bcherny
Copy link
Author

bcherny commented Jul 8, 2016

@anodynos I'm not sure what you mean, exactly. What's an inner body? And properties we can compile fine:

class A
  @b = 1
  constructor: ->
    @c = 3

Compiled to ES5 today:

var A;
A = (function() {
  A.b = 1;
  function A() {
    this.c = 3;
  }
  return A;
})();

Compiled to ES2015 classes:

var A;
A = (function() {
  A.b = 1;
  class A {
    constructor() {
      this.c = 3;
    }
  }
  return A;
})();

@anodynos
Copy link

anodynos commented Jul 8, 2016

Well, everything is possible :-)
I mean properties as instance members, that actually become part of the prototype, not static ones like A.b above (that belong to the class A). So

class Thing
  b: 'nice'
Thing = (function() {
  function Thing() {}
  Thing.prototype.b = 'nice';
  return Thing;
})();

These dont exist in ES6 - of course can be emulated with a getter/setter, but its a breaking change...

Inner bodies also dont exist is ES6, eg

class Thing
   somePrivateFunction = -> 
   someArbitraryCode()

The sad thing is that current CoffeeScript 1.x has extremely limited support for ES6 and everyone is on the ES6 train right now :-(

@connec
Copy link
Collaborator

connec commented Jul 12, 2016

I had thought about executable class bodies as one of the really neat things about CS that I would sorely miss. You could achieve the same effect, though. Also, ES6 still supports prototype as normal.

class A
  counter = 0
  # other executable class body statements

  # Non-function prototype property
  b: 'nice'

  constructor: ->
    @id = ++counter
let A = (() => {
  let counter

  class A {
    constructor () {
      this.id = ++counter
    }
  }

  // Non-function prototype property
  A.prototype.b = 'nice'

  (function () {
    counter = 0
    // other executable class body statements
  }).call(A)

  return A
})()

The main issue with this is that it does spread the code out a bit from how it was written (e.g. all functions would be 'hoisted' into the class definition).

@yogurt1
Copy link

yogurt1 commented Aug 7, 2016

Dont break CS class, just add new word _class or something else that implements ES6 class
Example:
class A {} #CS6 Class
_class B {} #ES6 Class

@bcherny
Copy link
Author

bcherny commented Aug 7, 2016

@yogurt1 a flag seems cleaner to me. With your approach, you are coupling the concerns of writing code and setting the compiler's target.

@michaelficarra @vendethiel Now that we've talked over the semantics of this change (the differences with what we have today are not large), would either of you guys be interested in patching this?

@brianmhunt
Copy link

A related note: The functions of ES6 classes are not enumerable.

To properly extend an ES6 class in Coffeescript one has to use Object.getOwnPropertyNames, but Coffeescript presently uses for (var key in parent).

e.g.

class ES6Class {
   static foo() {}
}
class CSClass extends ES6Class {}

The expected behaviour is for CSClass to have CSClass.foo.

Related discussion lodash/lodash#649.

@GeoffreyBooth
Copy link
Collaborator

Cross-posting; please see coffeescript6/discuss#22 (comment) and #4330

@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commented Jan 23, 2017

Our solution for this is CoffeeScript 2 and its support for the ES class keyword. See #4354.

As I understand things, it isn’t possible to extend an ES class without using the class keyword, which means that extending ES classes is a feature that can never come to CoffeeScript 1.x. If you can demonstrate that I’m incorrect about this (and I didn’t see anything useful in the Angular link) please post a comment and we can reopen this ticket.

Solutions that aren’t acceptable:

  • A flag to tell CS1 to use the CS2 class implementation. If you want ES classes, you just need to use CS2.
  • “Opting in” to using the class keyword just in this case. This would mean mixing the CS2 class implementation with the CS1 class implementation in the same codebase, which would be a nightmare to manage; and since the CS2 implementation has a few breaking changes from the CS1 one, different classes in the same project would behave differently based on whether they were extending ES classes or not.
  • Extending an ES class to create a CS1 function-style “class,” but this class behaves differently or has limitations as compared to a non-extended class.

We hope that CoffeeScript 2 will have so few breaking changes that there should be little hesitation in upgrading, or starting a new project with it. We hope that the better support for ES2015+ outweighs the hassle of needing to attach Babel or another transpiler if you need to generate JavaScript for browsers.

@bcherny
Copy link
Author

bcherny commented Jan 23, 2017

@GeoffreyBooth What's the roadmap for CS2? When (very roughly) can we expect it to be a replacement for CS?

@GeoffreyBooth
Copy link
Collaborator

@bcherny You can start using the 2 branch if you want, though it hasn’t even been released as alpha yet; use at your own risk. Updated docs.

There’s no timeline, but we’re close to an initial alpha release. See https://github.com/coffeescript6/discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests