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

Should static properties be inherited? #30

Open
samreid opened this issue Oct 23, 2017 · 13 comments
Open

Should static properties be inherited? #30

samreid opened this issue Oct 23, 2017 · 13 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Oct 23, 2017

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
Copy link
Contributor

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
Copy link
Member Author

samreid commented Jan 17, 2018

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
Copy link
Contributor

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
Copy link
Member Author

samreid commented Jan 17, 2018

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)

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 17, 2018

@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
Copy link
Member Author

samreid commented Jan 18, 2018

// ES6
class A {
  constructor() {
    console.log("hello");
  }

  static staticMethod() {
  }
}

class B extends A{}

B.staticMethod // returns the method

@samreid
Copy link
Member Author

samreid commented Jan 18, 2018

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
Copy link
Member Author

samreid commented Jan 18, 2018

image

@samreid
Copy link
Member Author

samreid commented Jan 18, 2018

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.

@samreid
Copy link
Member Author

samreid commented Jan 18, 2018

@jonathanolson will investigate.

@jonathanolson
Copy link
Contributor

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
Copy link

@jonathanolson that seems fine to me.

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

@ariel-phet ariel-phet removed their assignment Jan 23, 2018
@samreid
Copy link
Member Author

samreid commented Jan 24, 2018

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.

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

No branches or pull requests

4 participants