Page MenuHomePhabricator

Static methods can't know which class they belong to or which class they're called with
Open, MediumPublic

Description

Static methods in OOjs can't know which class they belong to, or which class they're called with (unlike regular methods, which can use this.constructor).

Use case:

  • Imagine you have a set of classes, with a common ancestor, that can be serialized to a string and reconstructed from such string.
  • You want to implement Animal#serialize and Animal.static.unserialize, and call them like dog = new Dog( … ); string = dog.serialize() and dog = Dog.static.unserialize( string ), and you also want to verify that the passed string actually unserializes to an instance of this class (in this case, Dog and not Cat).
  • However, the Animal.static.unserialize method, which is being called here, doesn't know whether it's being called on Dog.static, Cat.static or Animal.static.

You can invent a different API (like dog = Animal.unserialize( string, 'Dog' ), or dog = new Dog().unserialize( string )), but the original one is by far the prettiest.

Do we want to support this? I played with it and it seems simple to provide a class property on static objects, such that Dog.static.class === Dog, which could be accessed as this.class from static methods.

Event Timeline

matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex added a project: OOjs core.

The following patch seems to work.

1diff --git a/src/core.js b/src/core.js
2index 33333dc..42b8b67 100644
3--- a/src/core.js
4+++ b/src/core.js
5@@ -20,7 +20,11 @@ var
6 * @param {Function} fn
7 */
8 oo.initClass = function ( fn ) {
9+ if ( fn.static && fn.static['class'] !== fn ) {
10+ throw new Error( 'Class has mismatched static object' );
11+ }
12 fn.static = fn.static || {};
13+ fn.static['class'] = fn;
14 };
15
16 /**
17@@ -87,6 +91,7 @@ oo.inheritClass = function ( targetFn, originFn ) {
18 // Extend static properties - always initialize both sides
19 oo.initClass( originFn );
20 targetFn.static = Object.create( originFn.static );
21+ targetFn.static['class'] = targetFn;
22 };
23
24 /**
25@@ -134,7 +139,7 @@ oo.mixinClass = function ( targetFn, originFn ) {
26 oo.initClass( targetFn );
27 if ( originFn.static ) {
28 for ( key in originFn.static ) {
29- if ( hasOwn.call( originFn.static, key ) ) {
30+ if ( key !== 'class' && hasOwn.call( originFn.static, key ) ) {
31 targetFn.static[key] = originFn.static[key];
32 }
33 }

Would also be useful for inheriting static constructors. Obviously overriding static constructors may be needed sometimes, but generally it's a pain that you are forced to always override them with mostly identical implementations because there's no way to specify the class dynamically, it must be hard coded.

gerritbot subscribed.

Change 191100 had a related patch set uploaded (by Bartosz Dziewoński):
Provide .static.class property on OOjs classes

https://gerrit.wikimedia.org/r/191100

Patch-For-Review

If I understand correctly, this task is asking for a way to access the current class from a static method. Given Dog.static.unserialize is not overridden, the code authored as Animal.static.unserialize would get a reference to Dog. A bit like Late Static Bindings in PHP (e.g. akin to static::, not self::).

Given how OOjs looks right now, this seems like the quick solution. But I'm worried where it takes us.

For our use case adding a .class (or .self) property looks like an abuse of inheritance and 3 levels of indirection for something that is really just asking for a parameter of sorts

// 1. Generic
function unserialize() { Ctor, data ) {
  return new Ctor( data );
}

unserialize( Dog, .. );
// 2. Proposed before
Animal.static.self = Animal;
Animal.static.unserialize = function ( data ) {
  var animal = new this.self( data );
};
Dog.inherits( Animal );
Dog.static.self = Dog;

Dog.static.unserialize( .. );
//-> Animal.static.unserialize
//-> this.self
//-> Dog

That looks a bit crippled. Maintaining an additional property. Passing the value around a lot only to end up at the class the code already referenced.

While it'll take a minor refactoring (non-breaking), the following would be a lot cleaner and gets rid of some technical debt.

// 3. Proposed alternative
Animal.unserialize = function ( data ) {
  var animal = new this( data );
};
Dog.inherits( Animal );

Dog.unserialize( .. );
//-> this
//-> Dog

I support one of two outcomes.

  1. We use ['class'] to access this new property for now (since IE8 can't handled reserved words as property names) and in environments where IE8 isn't an issue (such as VisualEditor today, or the future in general) we can use .class. This is the least invasive and will cause no breaking changes today or in the future.
  1. We dispense with the "live inheritance" model, and go back to copying static properties and methods inside of inheritClass and mixinClass, and attach them directly to the constructor, such that we don't need this property because we have "this". Timo argued originally for the prototypal inheritance of static properties and methods, but later saw that it wasn't all he had hoped for. I'll let him elaborate on his current position.

I have no strong feelings one way or the other.

Krinkle's proposal requires copying the property/method references, rather than relying on prototypes to do the work for us, which isn't as neat conceptually as the current approach – but on the other hand, we already do this for mixins. And lack of .static in calls does look tempting.

Assuming that Krinkle's version is not going to have a patch written soon, I'd prefer if we went with .static.class for now – if we opt for the rewrite, it shouldn't be much more expensive to support than .static itself (one more statement everywhere we set .static, I guess), and I'd like to start using the feature in OOUI.

Using .static.class is the only way to go without a major version change in OOjs, because it's a major breaking change to get rid of .static or start copying things from the parent constructor.

I'm open to going the #2 route that Timo has suggested in version 2 of the library.

matmarex triaged this task as Medium priority.Feb 19 2015, 4:54 PM

Personally, I favour prototypal inheritance using objects and makers (e.g. no classes, constructor prototypes, new, or this whatsoever), but that's not where the language is headed. We'd need a pretty strong argument to go against that (at cost of interoperability with other libraries, and collaboration with the industry and other contributors to fundamentally change our underlying model). I do think the classical system has lots of conceptual flaws and limitations. Not using that could save a great deal of technical debt and human error. But.. that's for another day to redesign and think about. Perhaps OOjs v2.0.

Within the class model it makes sense to follow ES6. Copying members would only reinforce the classical model we're using in OOjs – which is that classes shouldn't be mutable. Instead one extends or uses a mixin. The for-loop to copy is just an implementation detail – one that might be faster than Object.create. (jsperf)

Copying may look weird, but it's very natural. Remember it only copies references, not values. Most OO libs I've seen do this in one place or another (for static, methods, or both). In fact, VisualEditors' class system, too, did this (before it became OOjs). It didn't support static members and was actually copying the prototype (!) members. I fixed that when I joined the team.

Of course, it being JavaScript (ignoring ES5 freeze and seal) for quick debugging one can still modify a class and instantly affect all existing instances. (Per class, not of per parent class.) This is a good thing. @matmarex also pointed this out on IRC earlier as a useful feature. In fact, because we're only talking about static members (not prototype) it wouldn't affect instances anyway.

Note that one doesn't actually have to do a copy. The way ES6 internally implements inherited static members for classes essentially works by setting the internal prototype of the constructor function. We can do that as well.

function Foo(x) {
  this.x = x;
}
Foo.is = function ( target ) {
  return target instanceof this;
};
Foo.prototype.walk = function ( add ) {
  return 'walking' + ( add ? ' ' + add : '' );
};

function FooBar( x, y ) {
  FooBar.super.call( this, x );
  this.y = y;
}
// "inheritClass"
FooBar.__proto__ = FooBar.super = Foo;
FooBar.prototype.__proto__ = Foo.prototype;

FooBar.isNot = function ( target ) {
  return !this.is( target );
};
FooBar.prototype.run = function () {
  return this.walk( 'faster' );
};

var f = new Foo( 'A' );
var fb = new FooBar( 'A', 'B' );
f.walk(); // "walking"
fb.walk(); // "walking"
fb.run(); // "walking faster"
Foo.is( f ); // true
Foo.is( fb ); // true
FooBar.is( f ); // false
FooBar.is( fb ); // true

This is supported in all our Grade A browsers. Technically, __proto__ is non-standard, but so were many other aspects of the language and the DOM (e.g. .innerHTML). All browsers implement it, and the standards are behind on this one. __proto__ has been standardised in ES6.

The only catch is that proto was not supported in IE8. There's no polyfill for it. I'd recommend we go with copying for the interim.

@TrevorParscal: As for breaking compatibility, I'd recommend we ship this now and keep transferring .static for existing classes (simple if-check). That way we can start using it today, and gradually move over existing stuff as we see fit. Or, if we choose to, we can migrate everything at once (in OOUI/VE). Either way, we can keep supporting the old property.

@Krinkle __proto__ was not standardized in ES6 (except in a "deprecated browser compatibility" appendix), however Object.setPrototype and Object.getPrototype are, which do the same thing as setting/getting __proto__.

But if I understand correctly, @Krinkle's proposal is that the static method would use this instead of this.class to refer to its type, regardless of how that is implemented, and Foo.static would just be an alias for Foo. Right?

@cscott: That's not what I had in mind, though that's in interesting idea.

I was thinking we'd keep them separate and migrate on our own terms (and drop in 2.0, though we can stop using it before then). That way there won't be a subtle change in behaviour and no clashing or implicit merging.

For the use case where we need a static method that is able to find its own late-static-binding self, can be defined as proper static method now while existing code using the .static container is unaffected.

So, how should OO.ui.Element.static.infuse() be written?
Option 1:

var obj = OO.ui.Element.static.unsafeInfuse(typeOrId);
if ( !( obj instanceof this['class'] ) ) { // how it is now
   throw new Error('Bad type check or whatever');
}
return obj;

Option 2:

if ( !( obj instanceof this ) ) { // Element.static = Element

Option 3:

if ( !( obj.constructor.static instanceof this ) ) { // oh, that's pretty evil

Option 4 (if I'm understanding you correctly):

OO.ui.Element.infuse = function( typeOrId ) { // no 'static', won't the users be surprised
  var obj = OO.ui.Element.unsafeInfuse( typeOfId );
  if ( !( obj instanceof this ) ) { // does have something nice about it
    throw new Error();
  }
  return obj;
}

or something else?

So, how should OO.ui.Element.static.infuse() be written?
Option 1:

var obj = OO.ui.Element.static.unsafeInfuse(typeOrId);
if ( !( obj instanceof this['class'] ) ) { // how it is now
   throw new Error('Bad type check or whatever');
}
return obj;

I don't know why this code is currently in the repo (albeit commented out). static['class'] has not been part of any OOjs version released or otherwise.

Option 3:

if ( !( obj.constructor.static instanceof this ) ) { // oh, that's pretty evil

I don't think that works. It would have to be obj.constructor.static === this. Assuming the code runs in a function attached to ClassName.static and obj is an instance of that class.

Option 4 (if I'm understanding you correctly):

OO.ui.Element.infuse = function( typeOrId ) { // no 'static', won't the users be surprised
  var obj = OO.ui.Element.unsafeInfuse( typeOfId );
  if ( !( obj instanceof this ) ) { // does have something nice about it
    throw new Error();
  }
  return obj;
}

Why would users be surprised?

This is the way static methods work in other programming languages with a classical system. "static" is never part of the identifier name or path; rather an internal classifier or natural concept (a property intended to be accessed as property of the class object instead of as property of a class instance). Element::infuse as opposed to $element->infuse (in PHP syntax). It also applies to the class model as first introduced to JavaScript in ES6. The .static object used by OOjs is just a workaround we came up with. It carries no native meaning. I'd say removing this will make things more natural, intuitive and understandable for other developers and remove the confusion that stems from non-standard in-house inventions. Matching how many other libraries provide static methods (e.g. jQuery).

The users would be surprised because all of the other static methods in OOjs UI are currently accessed via 'static'. So this is a big change for current users of OOjs (certainly in terms of lines of code affected), regardless of whether or not its a common pattern in other languages. As I understand it, the difference between Option 2 and Option 4 is that in option 4 the infuse method -- unlike all the other static/class methods -- *must* be invoked without .static or it will break.

And I'm specifically talking about the short-term here. Sure, in OOjs UI v2.0 all of the static nonsense is gone. But how can we make element infusion typesafe in OOjs UI 0.9?

Why don't we make Class.static -> Class for backwards compatibility, and go with Timo's suggestion of just using copying of properties from Class to SubClass?

The users would be surprised because all of the other static methods in OOjs UI are currently accessed via 'static'. So this is a big change for current users of OOjs (certainly in terms of lines of code affected), regardless of whether or not its a common pattern in other languages. As I understand it, the difference between Option 2 and Option 4 is that in option 4 the infuse method -- unlike all the other static/class methods -- *must* be invoked without .static or it will break.

I know what you mean, but it's not a matter of invoking it with or without. They would be stored in different containers and separate entities. For example, it is also not confusing that some methods are invoked with this or .prototype and others as Class. It's the difference between static methods and instance methods. Using the option I proposed we'd have a third bucket of proper static properties. And gradually phase out the old one.

Anyway, I was proposing it for the user's good. Not my own. Specifically so that existing code bases and properties will not suddenly change from being inherited to being copied. I fear that is a subtly breaking change.

However if you and @TrevorParscal are both confident that existing code will work fine. Then I agree there's no point in maintaining a separate bucket. We can just upgrade it to the new behaviour and keep .static as alias only.

I like @TrevorParscal's suggestion.

It's possible some code might break -- it should probably be a major version bump, just to be safe -- but any code that breaks was doing things it likely shouldn't have been doing in the first place.

For example, define`A`, add some methods to A.static, then have B inherit from A, then later add some more methods to A.static. This could be done inadvertently by a poor ordering of modules in your bundling tool.

But it's certainly not considered "best practices" to split up the definition of A.static in this way. And the fix is most likely a straight-forward reordering of modules.

In an ideal world, we wouldn't care about IE. But in the world we actual live in, I think copying properties is the best way forward.

We can also add copying without changing static at all. Then, people can start moving functions over.

Aliasing static to Class has the side effect, as @cscott has mentioned, that static using copying instead of prototypal inheritance so latent additions will behave differently.

Aliasing also has the side effect of changing the value of "this" inside static methods. This is evil, and we should have people make a deliberate choice to take advantage of the new static property copying feature and migrate toward the better solution on purpose.

So, I propose we add static copying and leave the static inheritance as it is. Deprecate .static and in v2 remove it.

@Trevor adding copying without changing static is the thing I was objecting to in option 4 above. I don't think that's a good idea. Having two different methods to invoke static methods is a bad idea. And worse, it means we can't update libraries to move methods to their 'proper' places without breaking all the users of the libraries as well. That's a bad migration strategy.

this inside static methods has always pointed to Foo.static. This remains the case. It just also points to Foo. I think this is the best of both worlds.

We can still deprecate .static, and start recommending that folks use Foo.method instead of Foo.static.method. But the change can be gradual; in v1 both syntaxes will be equivalent.

matmarex changed the task status from Open to Stalled.Mar 12 2015, 2:48 PM

I agree. Let's alias Class.static to Class and change the implementation to do copy. When we decide Wikimedia-wide to go ES5+ only for the JS run time (won't be long now), we can evaluate changing to live inheritance using __proto__ assignment (after considering its performance, which looked better than copying in my initial testing, but will revisit in the future).

Change 191100 abandoned by Bartosz Dziewoński:
Provide .static.class property on OOjs classes

Reason:
Better ideas were proposed on the task. To be reimplemented.

https://gerrit.wikimedia.org/r/191100

matmarex changed the task status from Stalled to Open.Mar 28 2015, 2:04 AM
matmarex removed a project: Patch-For-Review.
matmarex set Security to None.

Change 200279 had a related patch set uploaded (by Bartosz Dziewoński):
Alias Class.static to Class itself and copy static properties rather than inherit

https://gerrit.wikimedia.org/r/200279

I tried that – see patch above – and it turned out to be too good to be true.

ES6 defines a Function#name property, which is not writable. OOjs's Factory requires that objects registered with it have a .static.name property. These two turn out to conflict, which would make this a breaking change (we'd have to rename our 'name' property).

We could perhaps try leaving .static alone for now and just adding copying of class properties. We would still need to rename 'name' at some point, and we'd have two parallel systems.

Aye, right. Good catch.

Our bucket of more-or-less reserved names used to be limited to ES5 Object (e.g. hasOwnProperty, toString, "constructor"; and "watch" in Firefox) – which remained the same in ES6.

It will now also include ES5 Function (e.g. "length", "prototype", "apply", "bind") and indeed the ES6 Function "name" property. (Which browsers have had for a while but got standardised in ES6). All of these are either inherited (naturally shadowed by local property) or writable (can be assigned a new value) or configurable (re-declare property as writable). However re-declaring length and name is somewhat of an anti-pattern, similar to polluting global prototypes. May lead to unexpected behaviour when working with other frameworks. E.g. javascript loggers (Raven, Sentry), test frameworks (QUnit, Karma), or code overage (Istanbul, karma-coverage).

@matmarex Maintaining both is what I originally proposed. @cscott convinced me it's okay to replace the old with the new, but I think this is a good reason to try that after all. The current system for back-compat and existing code, the new one to migrate toward. Even if eventually unused, we can post-pone its removal until a OOjs 2.0.

Change 204048 had a related patch set uploaded (by Krinkle):
core: Support static properties on constructor object

https://gerrit.wikimedia.org/r/204048

Change 200279 abandoned by Bartosz Dziewoński:
core: Alias Class.static to Class and copy static properties

Reason:
Superseded by Krinkle's patch.

https://gerrit.wikimedia.org/r/200279

I'm not a fan of the new patch. At the very least, I think the commit should do a better job documenting what syntax it is recommending and/or deprecating.

In the new scheme, as I understand it, Foo.static.bar and Foo.bar are *not* aliased. So which are we recommending authors use?

If we're recommending that users begin to use Foo.bar, what's the migration strategy?

Is that idea that libraries like OOjs UI will manually alias all of its Foo.static.bar methods to Foo.bar, so that users can begin to use the newly-recommended syntax?

I thought the problem with Foo.bar in general is that it prevents you from using Foo.length() or Foo.name()...

If you want to make a clean break, I think @matmarex's original proposal of using Foo.static.class == Foo was a reasonable one for v1, and then shift to Foo.bar in v2.

Please tell me how we write Element.infuse (or Element.static.infuse), and how it is supposed to be used. That's what this task is all about, after all. Is there going to be a random mix of Foo.static.bar and Foo.bar methods in OOjs UI, and the user of the library has to remember which is which?

I'm not a fan of the new patch. At the very least, I think the commit should do a better job documenting what syntax it is recommending and/or deprecating.

OK. Should we mark them as deprecated right away? Seems a bit early. The systems are intended to co-exist for a while. There's no rush in removing the static container. It's cheap to maintain. Removal would be intrusive and costly maintenance wise for all existing code.

In the new scheme, as I understand it, Foo.static.bar and Foo.bar are *not* aliased.

Yes. That's what this task is about. Class.static.key is the old. Class.key the new. We're introducing this feature in the current release cycle so that authors have an easy and calm migration path. If there has been enough time when 2.0 or 3.0 comes along, we can remove it.

It's up to authors to gradually migrate their internal use. That can easily be done at once for each component in a single commit.

Regardless, they can start using the new system right away for new properties. Especially (which is what this task is for in the first place) if new properties need a late-static bound reference to the host class (via this) they can use this right away.

For public interfaces authors can choose whether they want to support both and how. That should be a very limited surface.

A small number of essential leaf classes may end up containing both types of properties (indirectly through mixin or inheritance). The API surface being mixed is somewhat confusing but inevitable and already the case since not everything uses .static even now.

Change 204048 abandoned by Krinkle:
core: Support static properties on constructor object

Reason:
Per T89721. Bartosz's solution (alias .static) is viable after all. We just need to migrate the reserved ".name" property and the likes first. Which is inevitable anyway. That is an annoying blocker to have to do first, but it seems that static properties isn't such a desperate need. But it's worth because letting the systems co-exist would confuse users a lot and make migration hard.

https://gerrit.wikimedia.org/r/204048

@Krinkle and I discussed this on IRC. I'm going to summarize the result, hopefully I'll get it right.

We agree that, short of the reserved keys like .name, aliasing Class and Class.static by reference is an ideal solution.

So, the proposal is to migrate reserved keys over the next month or two. And then ship the alias solution.

If constructor properties can't wait, we can ship "copying constructor properties" with a disclaimer that use should be limited and not be migrated towards, and *then* migrate reserved keys, and *then* flip from copying to aliasing. But we don't currently think the need is that urgent.

It's probably mostly Function.name and Factory which are at issue; we'll do a soft migration there: check Class.static.<notName> as well as Class.static.name for a while and deprecate .name. There might be other places where other properties of Function are at issue; according to MDN, arguments, arity, caller, displayName, length, name, prototype, apply, bind, call, isGenerator, toSource and toString are defined. We should probably open a tracking bug if it turns out that other names need to be migrated.

Krinkle changed the task status from Open to Stalled.Apr 21 2015, 6:41 PM
Krinkle removed a project: Patch-For-Review.
Krinkle removed a subscriber: gerritbot.

There might be other places where other properties of Function are at issue; according to MDN, arguments, arity, caller, displayName, length, name, prototype, apply, bind, call, isGenerator, toSource and toString are defined. We should probably open a tracking bug if it turns out that other names need to be migrated.

It looks like after ES6, these don't seem to exist anymore even in Firefox:

  • arity
  • displayName
  • isGenerator
  • toSource

And these throw exceptions by default if accessed on native class functions:

  • arguments
  • caller

Note that despite throwing by default if read or written on a class function object, they are allowed to be (re)declared as static members in which case the thrower isn't installed.

js
function Foo() {}
Foo.caller; // null
Foo.caller = 'x';
Foo.caller; // "x"

class Bar {}
Bar.caller; // TypeError
Bar.caller = 'x'; // TypeError

class Baz {
  static caller = "x";
}
Baz.caller; // "x"

So for our practical purposes, it seems we are able to use these.

As for the other reserved name, the same applies to native ES6 classes (ref T284935), so I think that's just something we're gonna have to live with and phase out any conflicts we think are problematic, as part of T96640.

class Foo {
  static name = 'foo-dialog';
  static length = 'medium';
}

Foo.name; // "foo-dialog"
Foo.length; // "medium"
Krinkle changed the task status from Stalled to Open.Nov 2 2023, 4:49 PM