[meta] Use the class keyword in JavaScript code

[mjethani]

Background

We have "classes" in our JavaScript code, usually defined in the following manner (example from the adblockpluschrome repo):

function PageMap()
{
  this._map = new Map();
}

PageMap.prototype = {
  get(page)
  {
    return this._map.get(page.id);
  },

  set(page, value)
  {
    this._map.set(page.id, value);
  }
};

We could instead use the class keyword here to get the same result:

class PageMap
{
  constructor()
  {
    this._map = new Map();
  }

  get(page)
  {
    return this._map.get(page.id);
  }

  set(page, value)
  {
    this._map.set(page.id, value);
  }
}

Note that there are some good arguments against the use of classical inheritance in JavaScript:

• ​A curated list of resources on why ES6 (aka ES2015) classes are NOT awesome
• ​Rob Pike, the designer of Go, on why the language does not have type-based inheritance

This ticket is not about switching over wholesale to a classical inheritance model but rather about using the class keyword as syntactic sugar where we already have something that resembles a "class", as in the above example.

Ticket	Status	Resolution	Summary	Component	Owner
#6741	closed	rejected	Use ECMAScript 2015 classes	Core	mjethani

[agiammarchi]

My 2 cents. This is a good choice/decision, and the linked list, even if interesting, is at least 2 years old.

Meanwhile JS moved on and developers either switched to functional programming patterns or adopted classes.

ES2015 classes are also not just syntactic sugar, but the only way to extends builtins when/if needed.

// fix the ambiguous Array constructor
class List extends Array {
  constructor(...args) {
    super().push(...args);
  }
}

That is a basic example, but there is more. Classes works very well when it comes to composition/mixins, in a way easy to spot through instanceof, as example.

Take this gist, as example: ​https://gist.github.com/WebReflection/f1666535e732ae254c8666b783cbcefb

You can start using one or more mixins right away.

Example, given the following mixin definition:

// helpers
const emitter = Symbol();
const wm = new WeakMap;

// class based mixin definition
const Emitter = Super => class extends Super {
  get [emitter]() {
    return wm.get(this) ||
            wm.set(this, Object.create(null)).get(this);
  }
  on(name, callback) {
    const listeners = this[emitter];
    if (!(name in listeners)) {
      listeners[name] = new Set;
    }
    listeners[name].add(callback);
    return this;
  }
  removeListener(name, callback) {
    const listeners = this[emitter];
    if (name in listeners) {
      listeners[name].delete(callback);
    }
    return this;
  }
  emit(name, ...args) {
    for (const fn of this[emitter][name] || []) {
      fn.apply(this, args);
    }
  }
};

One can use it through the mix utility as such:

class MyParagraph extends mix(HTMLElement).with(Emitter) {}

customElements.define('my-p', MyParagraph);

const p = new MyParagraph;
p.on('test', console.log);
p.emit('test', 1, 2, 3);  // 1, 2, 3
p.removeListener('test', console.log);

p instanceof HTMLElement; // true
p instanceof MyParagraph; // true
p instanceof Emitter;     // true

More on that pattern here: ​http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/

In ABP UI we've also double-checked classes are available since Opera 36, despite what MDN was saying, indeed I've filed a PR to update that wrong information and it's been merged yesterday: ​https://github.com/mdn/browser-compat-data/pull/1366

AFAIK we also agree in ABPUI we should use class everywhere we mean to use classical inheritance or prototypal through new Fn(...)

[mjethani]

@agiammarchi thanks, I was looking for your opinion on this.

By the way:

const Emitter = Super => class extends Super {
  ...
};

Shouldn't this be:

const Emitter = Super => class Emitter extends Super {
  ...
};

Because MyParagraph.prototype would ideally have a name so if it is printed in the console it prints Emitter {} rather than HTMLElement {}, which is misleading.

[agiammarchi]

@mjethani it depends. If your expectation is to have a mixin name in the prototype, last comes, last serves.

If your expectation is to have a super name, knowing that it might has been decorated with one or more mixins, then you don't name mixins.

// what would you expect to find as name of MyParagraph.prototype ?
class MyParagraph extends mix(HTMLElement).with(Emitter, Swipable) {}

I would expect to have certain knowledge of the super, and know the instance/class enough to use its mixins powers.

Knowing just last of the possible mixins give me only one capability but no idea about the kind of the class.

In few words, it's somehow more important to know it's an HTMLElement than an instance capable of emitting events.

Does this make any sense?

[sergz]

I would like to add that

• using of classes should be synchrnozied among several projects, e.g. adblockpluscore, libadblockplus as well as including eslint, jsdoc, etc. Therefore I would propose to create a meta issue for that, which references a transistion for each project and that meta issue should be a blocking for #6425.
• let's move e.g. to a separate issue all these discussions/links/etc about using of the common sense when one is using class or any other language feature, perhaps even independently from language.
  BTW, List should not be inherited from Array, even MyArray would be better than List.

[agiammarchi]

BTW, List should not be inherited from Array, even MyArray would be better than List.

I don't understand this comment, specially within its bullet point, but it's not in my interest create anything that extends Array these days so ... pick the name you prefer :-)

[mjethani]

@sergz I have made this the meta issue now. I'll file a separate one for adblockpluschrome.

[mjethani]

Replying to agiammarchi:

// what would you expect to find as name of MyParagraph.prototype ?
class MyParagraph extends mix(HTMLElement).with(Emitter, Swipable) {}

I would expect to have certain knowledge of the super, and know the instance/class enough to use its mixins powers.

Yeah, so the thing is that these aren't really mixins but rather subclasses. A "mixin" would be when we copy the properties from the prototype of one class into the prototype of another class. Alas then instanceof wouldn't tell you if it's an emitter, for example, so I can see the use of this. On the other hand, it lies about the inheritance chain. I'd prefer personally if it honestly reported the prototype as Swipable {} in the above example.

So new MyParagraph().__proto__ would be Swipable {} and new MyParagraph().__proto__.__proto__ would be Emitter {} and so on.

[agiammarchi]

Replying to mjethani:

Yeah, so the thing is that these aren't really mixins but rather subclasses. A "mixin" would be when we copy the properties from the prototype of one class into the prototype of another class. Alas then instanceof wouldn't tell you if it's an emitter, for example, so I can see the use of this. On the other hand, it lies about the inheritance chain. I'd prefer personally if it honestly reported the prototype as Swipable {} in the above example.

So new MyParagraph().__proto__ would be Swipable {} and new MyParagraph().__proto__.__proto__ would be Emitter {} and so on.

As privately mentioned in IRC, after a second though, I agree with you having discoverable mixins while debugging the generic object is a superior/easier experience.

It's true that here mixins are done through classes and protypal chain, but that's also the strenght of this approach: both instance fields, static methods, and also, when needed, constructor(...args) initializations are features otherwise impossible to obtain via prototype mutation so I hope the patter, using named mixins, would be considered/adopted so that we can use it in UI too.

Regards

[mjethani]

@sergz by the way, are classes supported on all the platforms that adblockpluscore code is supposed to run on?

[agiammarchi]

FYI the mixin gist is now a 100% code covered npm module called endow [1] (mostly due lack of free names to describe mixins).

You can import it in all flavors (ESM, CJS, global util).

[1] ​https://www.npmjs.com/package/endow

[sergz]

Replying to mjethani:

@sergz by the way, are classes supported on all the platforms that adblockpluscore code is supposed to run on?

I'm not sure about some cases like opera and safari, but it seems it's supported in chrome, firefox and libadblockplus.

[agiammarchi]

Not sure if this helps but in ABP UI we officially support these browsers:

Chrome 49
Edge 15
Firefox 51
Opera 36

all of them supports classes in both strict and sloppy mode.