Page MenuHomePhabricator

Support OO.Factory class keys (ES6) in OOUI WindowManager
Open, Needs TriagePublic

Description

Example of problematic code

class MyDialog extends OO.ui.ProcessDialog { } 
MyDialog.static.name = 'MyDialog'; 
console.log(OO.ui.ProcessDialog.static.name); // => Surprise: "MyDialog"

What and why it's incompatible
OO.inheritClass does more than a class extends does: it additionally sets prototype chains on the static object on the constructor, and a super property to the parent class.

Consequently, by using ES6 class extends, the static object and super property will not work properly. (Note: this is different to the super keyword in ES6 classes!)

What to do
To make this compatible it is likely for us to introduce breaking changes. Either moving static properties to the constructor directly or create another wrapper around class extends (e.g. let MyClass = OO.finalizeInheritance(class extends OO.SomeClass{ ... })?)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I've been using this wrapper I wrote for my ES6 modern-syntax classes extending OOjs classes (example) to make them work like native OOjs classes:

function tweakOoJsClass(targetClass) {
  const originClass = Object.getPrototypeOf(targetClass);
  OO.initClass(originClass);
  targetClass.static = Object.create(originClass.static);
  Object.keys(targetClass)
    .filter((key) => key !== 'static')
    .forEach((key) => {
      targetClass.static[key] = targetClass[key];
    });
  targetClass.parent = targetClass.super = originClass;
  return targetClass;
}

Update: this is indended to convert static properties declared by the static keyword, which is not included in ES6 or currently supported by ResourceLoader. To circumvent this, you can either declare static properties in separate statements like MyDialog.name = 'myDialog'; and then run tweakOoJsClass() or use getters and an updated wrapper as I described in T358416#9599082.

This has already been addressed in OOjs core through these two tasks:

Can you try again with .key instead of .static.name and report back whether you can register and create your dialog through the chosen key?

This has already been addressed in OOjs core through these two tasks:

Can you try again with .key instead of .static.name and report back whether you can register and create your dialog through the chosen key?

The actual problem is not specific to the dialog -- it's the prototype chain not correctly set up on the static object. Other use cases involving the static object would have the exact issue.

@Diskdance I understand that. We made the decision to not awkwardly fit an old ES3 idea into ES6 code. Instead, we added support in OOjs to use the native ES6 static properties directly, so that neither you nor we have to workaround these issues.

In ES6, static members work like this:

class Foo {
  static key = 'myfoo';
  static something = 'else';
}

class Bar extends Foo {
  static key = 'myfoo';
}

Foo.key
//> "myfoo"
Foo.something
//> "else"

Bar.key
//> "myfoo"
Bar.something
//> "else" 

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static

I believe your example in the task description is a modified version of what we recommend on https://www.mediawiki.org/wiki/OOUI/Windows/Dialogs. If you follow that recommendation as-is, it will work.

The modification you made is incomplete, as it combines ES6 classes with old ES3 static properties. A native ES6 version would look like this:

class MyDialog extends OO.ui.Dialog {
    static key = 'myDialog';

    initialize() {
        super.initialize();
        this.content = new OO.ui.PanelLayout( { padded: true, expanded: false } );
        this.content.$element.append( '<p>A simple dialog window. Press Escape key to ' +
        'close.</p>' );
        this.$body.append( this.content.$element );
    };
}

When used with OO.Factory or OO.Registry, this works today.

When used with OO.ui.WindowManager, which builds on top of OO.Factory, this does not yet work today because OOUI hardcodes constructor.static.name without also checking constructor.key.

I'm untagging OOjs and Platform Team, because this is specifically about updating OOUI WindowManager to support ES6.

Krinkle renamed this task from OOjs classes are not fully compatible with ES6 class extends syntax to Support OO.Factory class keys (ES6) in OOUI WindowManager.Feb 25 2024, 6:25 PM

@Krinkle The same issue affects other classes and static properties though, and most of them don't have an alternative like .key for .static.name. For example:

class MyWidget extends OO.ui.Widget {}
MyWidget.static.tagName = 'details'; // Oops, this affects *all* widgets

So, I think it made sense to consider this a general issue with OOUI at least, rather than just WindowManager.

I suppose the solution for that would be to add the alternative properties and use them as this.constructor.foo everywhere that this.constructor.static.foo is used?

@matmarex Yeah.

If you can think of ways to ease the migration or generally, ideas welcome :)

I coudln't see a way to automate it at glance. In exiting code, OO.inheritClass() will generally be called before the static properties are set, so a one-time forward-compat copy to native static is not feasible (too early from that point in time).

What we could potentially, at least warn against incomplete migrations, That is, when a class is natively extended, somehow set Class.static to null with an own property. By shadowing the parent static, developers would avoid mistakes. I'd support that. Can you think of a way?

I couldn't see a way to automate it at glance. In exiting code, OO.inheritClass() will generally be called before the static properties are set, so a one-time forward-compat copy to native static is not feasible (too early from that point in time).

I think we'd have to handle it everywhere that we currently read from .static – instead of this.constructor.static.foo, I think we could write Object.hasOwn(this.constructor, 'static') ? this.constructor.static.foo : this.constructor.foo (this could be a helper method), and everything would just work with ES6 static properties. That doesn't seem too bad?

What we could potentially, at least warn against incomplete migrations, That is, when a class is natively extended, somehow set Class.static to null with an own property. By shadowing the parent static, developers would avoid mistakes. I'd support that. Can you think of a way?

As far as I know this isn't possible, there isn't a way to be notified when a class is extended. (But if it was, we could just set up .static "correctly", like we do in inheritClass().)

Maybe it would be possible to implement with property accessors, so that the magic would happen when .static.* is accessed for the first time. If it's possible, I wouldn't want to see it in production. ;)

The modification you made is incomplete, as it combines ES6 classes with old ES3 static properties. A native ES6 version would look like this:

class MyDialog extends OO.ui.Dialog {
    static key = 'myDialog';

    initialize() {
        super.initialize();
        this.content = new OO.ui.PanelLayout( { padded: true, expanded: false } );
        this.content.$element.append( '<p>A simple dialog window. Press Escape key to ' +
        'close.</p>' );
        this.$body.append( this.content.$element );
    };
}

When used with OO.Factory or OO.Registry, this works today.

It doesn't though, as static class fields (as opposed to methods) were introduced only in ES2022 (MDN's compatibility table for static is misleading, and I made the same mistake). In ES6, you either need to declare static fields in separate statements like MyDialog.name = 'myDialog'; or use getters like

    static get name() {
			return 'myDialog';
		}

I rewrote the wrapper function I posted in T358416#9574722 to adapt it to static properties declared like this. (It assumes that the class has no static properties with a getter whose output is not fixed, which is fairly unlikely.) Anyone can use it to declare ES6 subclasses of OOjs classes now.