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

Static class properties should be inherited #1900

Closed
sophiebits opened this Issue Sep 10, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@sophiebits

sophiebits commented Sep 10, 2015

In ES6 classes, static properties should be inherited. That is,

class Foo {
}
Foo.x = 5;

class Bar extends Foo {
}

console.log(Bar.x);

should log "5". This can be done by looping over the properties and copying, but that's subpar because updating the static properties on the superclass should update them on the subclass too. Babel sets __proto__ to achieve this:

https://github.com/babel/babel/blob/554fda00c179eea72121cafe4b90fac85c1bb244/packages/babel/src/transformation/templates/helper-inherits.js#L13

It doesn't look like the ES5 emitter makes any attempt to get the same behavior:

/** Generates the JS constructor for a class, ES5 style. */
private def genES5Constructor(tree: LinkedClass): js.Tree = {
implicit val pos = tree.pos
val className = tree.name.name
val isJSClass = tree.kind.isJSClass
def makeInheritableCtorDef(ctorToMimic: js.Tree) = {
js.Block(
js.DocComment("@constructor"),
envFieldDef("h", className, js.Function(Nil, js.Skip())),
js.Assign(envField("h", className).prototype, ctorToMimic.prototype)
)
}
val ctorFun = if (!isJSClass) {
val superCtorCall = tree.superClass.fold[js.Tree] {
js.Skip()
} { parentIdent =>
js.Apply(
js.DotSelect(encodeClassVar(parentIdent.name), js.Ident("call")),
List(js.This()))
}
val fieldDefs = genFieldDefs(tree)
js.Function(Nil, js.Block(superCtorCall :: fieldDefs))
} else {
genConstructorFunForJSClass(tree)
}
val typeVar = encodeClassVar(className)
val docComment = js.DocComment("@constructor")
val ctorDef = envFieldDef("c", className, ctorFun)
val chainProto = tree.superClass.fold[js.Tree] {
js.Skip()
} { parentIdent =>
val (inheritedCtorDef, inheritedCtorRef) = if (!isJSClass) {
(js.Skip(), envField("h", parentIdent.name))
} else {
val superCtor = genRawJSClassConstructor(
linkedClassByName(parentIdent.name))
(makeInheritableCtorDef(superCtor), envField("h", className))
}
js.Block(
inheritedCtorDef,
js.Assign(typeVar.prototype, js.New(inheritedCtorRef, Nil)),
genAddToPrototype(className, js.Ident("constructor"), typeVar)
)
}
val inheritableCtorDef =
if (isJSClass) js.Skip()
else makeInheritableCtorDef(typeVar)
js.Block(docComment, ctorDef, chainProto, inheritableCtorDef)
}

This means scala-js does not work with React 0.14+ which checks for a static flag on classes extending React.Component:

http://stackoverflow.com/questions/32509891/warning-react-component-classes-must-extend-react-component-0-14-rc1

@sophiebits sophiebits changed the title from When inheriting, static properties should be copied to Static class properties should be inherited Sep 10, 2015

@sjrd

This comment has been minimized.

Member

sjrd commented Sep 13, 2015

Hum, this is pretty annoying. Why would they even specify something like that in ES6 in the first place?

My main concern about this is that there is no standard way to implement this behavior in ES5.1. Neither __proto__ nor setPrototypeOf are standard in ES 5.1 (setPrototypeOf is standard in ES 6; and __proto__ is standard in web browser ES 6). Choosing to use one or the other, or both, would therefore make us not comply to ES 5.1 anymore.

It seems like a small thing, but so far we have successfully avoided to rely on even the slightest non-standard feature. Breaking this here, even for a "widely supported non-standard extension" such as __proto__, would set a precedent for breaking more things like that in the future, and I don't like that.

Note that "widely supported" does not include IE < 11. So any code relying on this just can't work in IE 10 and earlier (and no, this is not bashing against IE; on the contrary, IE was the only browser to not deviate from the standard, here; everybody else is to blame). It is also unclear whether Rhino supports it.

And that's even without even considering the fact that setting __proto__ or using setPrototypeOf causes severe performance issues on the target objects.

On the other hand, it seems we don't even have a choice, if frameworks like React start relying on this prototype chain behavior. Why would they do such a thing when there is no workaround on IE 10!?

@sophiebits

This comment has been minimized.

sophiebits commented Sep 14, 2015

My mistake, I didn't realize this wasn't supported in IE10. We'll reconsider for React 0.14 final.

@sjrd

This comment has been minimized.

Member

sjrd commented Oct 1, 2015

Cool, they changed React not to rely on static property inheritance anymore :) So I believe we can close this on our end.

Thanks React's team :-)

@sjrd sjrd closed this Oct 1, 2015

@sophiebits

This comment has been minimized.

sophiebits commented Oct 1, 2015

I still suggest that you at least copy over static properties at class creation time.

@sjrd

This comment has been minimized.

Member

sjrd commented Oct 1, 2015

Without a compelling use case needing inheritance but not the ability to define static members (as React requiring us to do so would have been), a solution to this problem should be part of #1902 anyway.

@sophiebits

This comment has been minimized.

sophiebits commented Oct 1, 2015

Okay, sounds great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment