phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Should static properties be inherited? #30

Open samreid opened 6 years ago

samreid commented 6 years ago

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:

class A {
    static int hello() {
        System.out.println( "anotehunahtoeu" );
        return 0;
    }
}

class B extends A {
}

public class Main {

    public static void main( String[] args ) {

        B.hello();

        System.out.println( "Hello World!" );
    }
}

and we saw that the static attributes are accessible from the subtype:

anotehunahtoeu
Hello World!

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?

jonathanolson commented 6 years ago

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 hasOwnProperty properties on the parent type (and set those on the child type) before setting the child's own static fields.

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?

samreid commented 6 years ago

I would expect RANDOM_CONSTANT to be available on the ChildType in the case you described. What do you think about this new case?

inherit( Object, ParentType );
inherit( ParentType, ChildType );
ParentType.RANDOM_CONSTANT = 4; // https://xkcd.com/221/
ChildType.RANDOM_CONSTANT; // is this undefined or 4?
jonathanolson commented 6 years ago

What do you think about this new case?

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.

samreid commented 6 years ago

I don't think we should go out of our way to make https://github.com/phetsims/phet-core/issues/30#issuecomment-358365144 work, just wanted to clarify the distinction between it and https://github.com/phetsims/phet-core/issues/30#issuecomment-358177331

pixelzoom commented 6 years ago

@samreid said:

We tested another popular (?) language to check the behavior:

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.

samreid commented 6 years ago
// ES6
class A {
  constructor() {
    console.log("hello");
  }

  static staticMethod() {
  }
}

class B extends A{}

B.staticMethod // returns the method
samreid commented 6 years ago

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;
samreid commented 6 years ago

image

samreid commented 6 years ago

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 https://github.com/phetsims/phet-core/issues/30#issuecomment-358177331

Mimicing ES6 behavior fully seems too complex and difficult for now.

samreid commented 6 years ago

@jonathanolson will investigate.

jonathanolson commented 6 years ago

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).

ariel-phet commented 6 years ago

@jonathanolson that seems fine to me.

I would say Gene Expression or States of Matter would be good candidates in terms of performance.

samreid commented 6 years ago

Here is the warning at the top of MDN:

image

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 setPrototypeOf (mostly through uncertainty about it, and unfamiliarity with it and how it is used by browsers).

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.