-
Notifications
You must be signed in to change notification settings - Fork 6
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
Should static properties be inherited? #30
Comments
It looks like this is a standard pattern, and it would be good (long-term) to support. An additional question is should the following get copied: inherit( Object, ParentType );
ParentType.RANDOM_CONSTANT = 4; // https://xkcd.com/221/
inherit( ParentType, ChildType );
ChildType.RANDOM_CONSTANT; // is this undefined or 4? as that pattern is also used throughout the code. It's also easier to support this, as we can just iterate over Additionally, there's a decently high chance of causing hard-to-notice breakage with this, so I'd highly recommend doing snapshot testing with it. Can probably add hooks to automatically detect when this breaks things in most cases. Thoughts? |
I would expect inherit( Object, ParentType );
inherit( ParentType, ChildType );
ParentType.RANDOM_CONSTANT = 4; // https://xkcd.com/221/
ChildType.RANDOM_CONSTANT; // is this undefined or 4? |
Presumably would be undefined with my latest proposal. There may be a way to get that to work (if we can properly set up the constructor's prototype). We may have the browser support for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf, but it's supposed to crater performance. Alternatively, there may be a better-performance way of doing that (but may require refactoring every single constructor creation). I'm skeptical that this would be worth the drawbacks. |
I don't think we should go out of our way to make #30 (comment) work, just wanted to clarify the distinction between it and #30 (comment) |
@samreid said:
I think we should be more interested in being consistent with how JavaScript typically handles this. For prototypal inheritance, I'm not certain that statics are inherited as they are in (for example) Java. And I also have no experience with the class features in ES6. Btw... I recall a discussion about this on Slack, circa the creation date of this issue. And I know that I had more to say about this at the time. But I can no longer access those comments, due to the 10K-message limit. |
// ES6
class A {
constructor() {
console.log("hello");
}
static staticMethod() {
}
}
class B extends A{}
B.staticMethod // returns the method |
Here's how Babel does it: class A{}
class B extends A{}
A.hello=123; "use strict";
function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }
function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }
function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }
var A = function A() {
_classCallCheck(this, A);
};
var B = function (_A) {
_inherits(B, _A);
function B() {
_classCallCheck(this, B);
return _possibleConstructorReturn(this, (B.__proto__ || Object.getPrototypeOf(B)).apply(this, arguments));
}
return B;
}(A);
A.hello = 123; |
This doesn't seem urgent, perhaps we can leave it as it is now. Or we could use copies like in phetioInherit and proposed in #30 (comment) Mimicing ES6 behavior fully seems too complex and difficult for now. |
@jonathanolson will investigate. |
Babel does use setPrototypeOf(). The warning at the top of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf is somewhat concerning, but it would probably be good to measure whether there is a performance decline with its use. @ariel-phet, would it be fine to build a one-off of a sim to test with a performance comparison? (What sim would be best to compare performance? Preferably something somewhat CPU-bound). |
@jonathanolson that seems fine to me. I would say Gene Expression or States of Matter would be good candidates in terms of performance. |
Here is the warning at the top of MDN: On the one hand, Babel decided it's safe enough to use for production code. But on the other hand, even if all browsers work fine (including performance) at the moment, we will have to remain vigilant to test browsers as new versions come out so we don't pick up a problem from this. I'm still skeptical about using Also, brainstorming: I wonder if we could/should have inherit return a new object (with the correct prototype) instead of modifying the passed in type. |
While @zepumph and I were investigating https://github.com/phetsims/phet-io/issues/1236 we created TNumberProperty and were surprised to find that it didn't inherit any of the static fields on TProperty. We tested another popular (?) language to check the behavior:
and we saw that the static attributes are accessible from the subtype:
Should we emulate this in our own inherit call? Have we just not needed it until now? If we add it, will it cause performance or other runtime problems?
The text was updated successfully, but these errors were encountered: