-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[CS2] Compile class constructors to ES2015 classes #4354
Conversation
@connec how does this compare with #4330? Is this a replacement, or are you breaking up the classes effort into multiple smaller PRs? I and @mrmowgli, at least, would like to also work on classes. I’ve invited you both to my repo, if you’d like to work on a branch there. Or perhaps you’d like to invite us to your repo? Or how would you propose collaborating on this? |
Sorry, to be clear, this supersedes #4330, and is the 'final' implementation of what was discussed there. namely:
Some sample of the output: class Title extends Component
@selfClosing = false
###
Initialise a `Title` component with some text.
###
constructor: (@text) ->
render: ->
super "<h1>#{@text}</h1>" var Title;
Title = (function(superClass) {
class Title extends superClass {
/*
Initialise a `Title` component with some text.
*/
constructor(text) {
super(...arguments);
this.text = text;
}
}
Title.__super__ = superClass.prototype;
Title.selfClosing = false;
Title.prototype.render = function() {
return Title.__super__.render.call(this, "<h1>" + this.text + "</h1>");
};
return Title;
})(Component); |
jsClass.placeholder.updateFragments result | ||
result | ||
|
||
#### Javascript Classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should call this “ECMAScript Classes” or ESClass
. The phrase “JavaScript class” for me implies a function
-based construct, like what’s generated by the CoffeeScript 1.x class
keyword; as opposed to the ES2015+ output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, will update.
I may be late to the party here (just got the link from the closing of #4330), but adding an implicit call to Since calling |
@svicalifornia nothing's been decided on that front yet, this is just what I thought was a sensible choice. To be clear: the implementation here will add an implicit call to I'm a fan of |
@connec I would like to add more tests to this. I can submit a pull request, or transfer your branch to my repo where others can collaborate on it. What is your preference? |
Sorry @GeoffreyBooth, I've been a bit swamped this week. I've added you as a collaborator as I'd like to stay fully in the loop for this change. |
I did some experiments using https://github.com/balupton/es6-javascript-class-interop. I forked it, patched it to work with the current version of Node and its dependencies, and swapped out its So what’s interesting is that in the original, 2 tests fail of 9:
In my version, 2 tests still fail, but one of them is different:
So basically, leaving CoffeeScript aside for a moment, ES5 “classes” cannot extend ES2015 classes. Hence, the CoffeeScript 1.x I guess this is the core of why we’re making this change in the first place. If ES5 functions could extend/inherit from ES2015 classes, then CoffeeScript 1.x’s classes wouldn’t need changing. But what, if anything, is lost by breaking downstream ES5 code’s ability to extend a CoffeeScript class? |
To take the test from the top of #4233, in CoffeeScript 1.x:
This branch:
So I think this should fix #4233 unless there’s something else I’m missing. |
@GeoffreyBooth yes, in fact the
// class B extends A
function B () {
(A.__proto__ || Object.getPrototypeOf(A)).apply(this, arguments)
}
|
OR
|
Clearly I've not had enough coffee. Babel's approach also only works with ES5 classes 😢 SO, that at least means this still seems the best approach for future compatibility. In the interest of providing options, we could compile to one of the ES5 variants in this thread: A = (function (superClass) {
function A () {
_this = new superClass(...arguments)
Object.setPrototypeOf(_this, A.prototype)
// compile `this`/`@` references to `_this`
return _this
}
Object.setPrototypeOf(superClass, A)
Object.setPrototypeOf(superClass.prototype, A.prototype)
return A
})(B) This would be a simpler compilation, and would make all the |
“This” meaning, the current implementation in this PR? Yes, I think so. Especially if by “future compatibility” you mean whatever other ES class features we decide to support in CoffeeScript. I can easily see adding support for Needing to be able to extend an ES2015 class or CoffeeScript class in ES5 code is probably such an edge case at this point that we shouldn’t worry about it. The only scenario I can think of is a Node module meant for general use, like If you can find a way to make all 9 of balupton’s tests pass, that would be heroic, and maybe such a solution could be merged into I want to add a few more tests, and invite others to look at this PR, and then I think it’s probably ready to merge. Then we can release our first alpha of CoffeeScript 2 (!) and discuss where to go from there. |
Okay, a little test. I took the simple class from the MDN example: class Polygon {
constructor(height, width) {
this.height = height;
this.width = width;
}
get area() {
return this.calcArea();
}
calcArea() {
return this.height * this.width;
}
} and rewrote it as CoffeeScript. Note that I just have to ignore the class Polygon
constructor: (@height, @width) ->
area: ->
@calcArea()
calcArea: ->
@height * @width Per this PR, this becomes: var Polygon;
Polygon = (function() {
class Polygon {
constructor(height, width) {
this.height = height;
this.width = width;
}
}
Polygon.prototype.area = function() {
return this.calcArea();
};
Polygon.prototype.calcArea = function() {
return this.height * this.width;
};
return Polygon;
})(); So Another thing we should discuss is the reliance on |
@@ -449,7 +449,7 @@ test "#1591, #1101: splatted expressions in destructuring assignment must be ass | |||
|
|||
test "#1643: splatted accesses in destructuring assignments should not be declared as variables", -> | |||
nonce = {} | |||
accesses = ['o.a', 'o["a"]', '(o.a)', '(o.a).a', '@o.a', 'C::a', 'C::', 'f().a', 'o?.a', 'o?.a.b', 'f?().a'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why it’s safe to remove C::
? This compiles to C.prototype
in both this PR and CoffeeScript 1.x. I see that the test uses this with a new class
, so presumably that’s why this test fails now; but why does it fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails because the test is attempting to assign to all those accesses, and the prototype
property of ES classes is non-writable.
@@ -738,21 +721,21 @@ test "#2599: other typed constructors should be inherited", -> | |||
ok (new Derived) not instanceof Base | |||
ok (new Base) not instanceof Base | |||
|
|||
test "#2359: extending native objects that use other typed constructors requires defining a constructor", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks fine, but do you mind explaining what changed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, from what I can tell, the main reason ES classes have been implemented the way they are is to fully support extending native objects. The test here therefore became invalid, i.e. you no longer need a constructor to extend native objects.
I decided to adjust the test to check that native objects could be extended with and without a constructor, rather than removing it.
That's exactly what I mean, sorry for not being clearer.
Unfortunately one of those failures is an incompatibility in ES itself (extending ES6 classes in ES5), so there's nothing we can do about that. The CS ones could be made to pass with some acrobatics, like above. I may take a stab at that, though I'm a bit worried the approach might interfere with 'factory' constructors that return non-instances.
You're right that this would be clearer. In fact in all cases except where dynamic keys are involved, we have the option of using |
Agreed. Sorry, I thought I wrote that but I guess I forgot. 😄 I feel like we’re likely to want to implement Thanks for your answers to my questions on the tests. I can’t think of any other tests to add. @jashkenas, @lydell, @mrmowgli, anyone else: any notes on this PR before we merge? |
@GeoffreyBooth it is in fact mostly dynamic keys. Without dynamic keys you can reorder the executable statements after the class definition without affecting the result. However, when dynamic keys are used, it's possible for the executable class body to modify the values of the keys, meaning the reordering could result in a different result. In the below example, the result will be different depending on whether the executable statements go before or after the class A
a = 'foo'
"#{a}": -> console.log 'foo called'
a = 'bar'
"#{a}": -> console.log 'bar called'
# class A {
# [`${a}`] () {
# console.log('foo called')
# }
# [`${a}`] () {
# console.log('bar called')
# }
# } Without going crazy with code flow analysis etc., it would be impossible to detect conflicts like that, so dynamic keys are incompatible with executable class bodies, and would have to be moved outside the For non-dynamic content there is no such conflict, and we can safely shift all the executable statements to before/after the class. class A
@mixin SomeMixin
f: -> @functionFromMixin()
# class A {
# f () {
# return this.functionFromMixin()
# }
# }
# (function () {
# this.mixin(SomeMixin)
# }).call(A) |
I guess you could just as easily say it’s mostly executable class bodies, or rather, a class that has both an executable class body and dynamic keys. Only in the case that has both do we need to use |
code.push @makeCode '}' | ||
code.push @makeCode "\n#{@tab}#{@name}.__super__ = #{@parent}.prototype;" if @parent | ||
|
||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method always returning an empty array, or am I confused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is! This is part of the 'hack' to hoist the constructor, whilst also ensuring it has the correct scope. Consider:
class A
@a = 1
In CS1.x this becomes:
A = (function () {
A.a = 1
function A () {}
return A;
})()
This works fine due to named function hoisting. Unfortunately class
es are not hoisted, so the same approach won't work. We also can't just move the node to the top, because the scope must correspond to the constructors position in the body, e.g.:
class A
@a = 1
a = 1
constructor: -> a = 2
The a
in the constructor should reference the outer scope, but if we move the class
above the @a = 1
then the constructor will have the wrong scope.
I couldn't find a mechanism in the compiler for dealing with this, so I resorted to compiling a 'placeholder' node in the target position, and leaving the constructor node in the source position. Once compilation is done, the placeholder is swapped out for the fragments compiled for the constructor.
In this function, rather than returning the fragments, they are being attached to the placeholder (@placeholder.fragments
) for later substitution, and so for this location no fragments are returned.
Regrettably this still leaves a blank line in the source where the constructor was. Now I think about it, the same approach could be used here: return a 'special' fragment that can be spliced out later.
If you have any suggestions for dealing with this (namely, compiling a node with a scope that's not available until further down the tree), or ways of clarifying what's here, I'd gladly hear them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, please add a comment near the returned empty array that explains this.
Look at my commit where I removed extra empty lines. Could you put some code there that helps with this?
This looks more or less fine to me — I just have a couple of comments:
|
In order to be usable at runtime, an extended ES class must call `super` OR return an alternative object. This check prevented the latter case, and checking for an alternative return can't be completed statically without control flow analysis.
So many edge cases... Another fun one with class A
class B extends A
constructor: (@a = (super ; @a)) -> The promise of class A {}
class B extends A {
constructor (a = ((() => {
var ref
ref = super(...arguments)
this.a = a // can't use `a` as it's not ready
return ref
})(), this.a)) {
}
} I guess the assignment of |
How I think we should handle those cases:
|
What about never allowing |
@lydell that's certainly an option, and it's realistically not likely to happen with idiomatic code. I honestly didn't expect it to work when I tried it, but figured I should see if we could support it before ruling it out. More than happy to go with |
I'm wondering why we need to allow function calls in parameter lists. As CoffeeScript's own compiler output shows, such code could always be rewritten such that the call is in the function body. It also doesn't strike me as something that happens often, so if we ban it we won't be breaking too much existing code. |
That's another option, though I could imagine a situation like: class User
nextId = do ->
currentId = 0
-> ++currentId
constructor: (@id = nextId()) -> Yet another option would be to reverting back to CS' old parameter defaults behaviour, which would work predictably in the above cases. Of the ones we've mentioned, my preference is either banning If we eventually support class properties in the same way Babel does, this issue would crop up again. I had hoped that someone from Babel would chime in on the issues I created with some wisdom, but I guess it's a pretty extreme edge case. |
Interesting. My tendency would be to ban it in this pull request, and possibly open another issue outlining the edge cases and discuss on CS6-Discuss. |
There are many edge cases when combining 'super' in parameter defaults with @-parameters and bound functions (and potentially property initializers in the future). Rather than attempting to resolve these edge cases, 'super' is now explicitly disallowed in constructor parameter defaults.
@-parameters can't be assigned unless 'super' is called.
OK, I've tidied up the rules around
I reckon this is ready to merge. No doubt there will be a few kinks in it, but we're not going to find them unless this gets used in some real projects (my yaml-js project is the only one using it atm). Once it's approved I would quite like to rebase it on to |
I think you're taking the right approach with disallowing the fancy option to start. We can always add it later if there's real demand for it. Thanks for seeing this through, and for the heroic effort, @connec. Looks good to merge to the 2 branch to me. |
I've rebased, for that fresh commit log feeling. @GeoffreyBooth and @mrmowgli - I've revoked your write access to prevent shenanigans, since it's ready to merge. If we need to make changes I can add you back in! |
To follow up on my example from above, the MDN var Polygon;
Polygon = class Polygon {
constructor(height, width) {
this.height = height;
this.width = width;
}
area() {
return this.calcArea();
}
calcArea() {
return this.height * this.width;
}
}; which looks pretty damn good. @connec, what remains to be done, that you wanted to follow up in a second PR? Improvements to |
OK, I've pushed what I believe is a more-or-less "ready" version of classes. I've adopted a similar approach to the previous PR, whereby valid methods are hoisted into an ES2015 class declaration, and other expressions are left as-is. I've also made it so that classes are compiled "bare", without an enclosing IIFE, where possible.
I've standardised the hoisting thing a little bit.
hoist
is now a method onBase
, meaning any node can be hoisted. Callinghoist
mutates the source node and returns a new target node, which should be put in the tree at the desired output location. When the target is compiled, it returns a fragments placeholder. When the source is compiled, it updates the target's fragments. Finally,Block#compileRoot
will expand all placeholder fragments before returning the result.I've not touched
super
outside constructors as that adds quite a bit of surface area and would be better handled separately, imo. The downside of this is that, for now, derived classes always need an enclosing IIFE. Handlingsuper
is something I'd be happy to look at as a subsequent PR, unless someone else wants to tackle it.There is one remaining annoyance, which is the assignment. This is needed to allow
Literal
to be referenced in the second example below (and throughout the CS source). We could remove this, which would mean that classes created in expressions can only be referenced by the result of the expression (e.g.exports.Foo = class Foo
could only be referenced asexports.Foo
). I've erred on the side of compatibility for now, as it's easy to take this out later if we so choose.I've not added any test, the following would be good to have. Any test contributions would be great.
@
params with/withoutsuper
, including errorssuper
, including errorssuper
in arrow functions in constructorssuper
and external constructorsAs a final note, the diff looks fairly large as I ended up rewriting the
Class
node, however a lot of the logic hasn't changed, its just been moved around to better line-up with the new usage of the node. I felt this was better than continuing to stretch the previousClass
out of shape. Apologies to reviewers.Supersedes #4330.