Opened 19 months ago

Last modified 16 months ago

#6482 new change

[meta] Use the class keyword in JavaScript code

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Unknown Keywords:
Cc: greiner, kzar, agiammarchi, sergz, sebastian, hfiguiere Blocked By: #6741
Blocking: #6425 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by 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:

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.


Change History (19)

comment:1 Changed 19 months ago by mjethani

  • Cc greiner kzar agiammarchi sergz sebastian hfiguiere added

comment:2 Changed 19 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 19 months ago by mjethani

  • Review URL(s) modified (diff)

comment:4 Changed 19 months ago by 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(...)

comment:5 Changed 19 months ago by 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.

comment:6 follow-up: Changed 19 months ago by 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?

Last edited 19 months ago by agiammarchi (previous) (diff)

comment:7 Changed 19 months ago by 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.

comment:8 Changed 19 months ago by 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 :-)

Last edited 19 months ago by agiammarchi (previous) (diff)

comment:9 Changed 19 months ago by mjethani

  • Component changed from Platform to Unknown
  • Summary changed from Use the class keyword as syntactic sugar to [meta] Use the class keyword

comment:10 Changed 19 months ago by mjethani

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:11 Changed 19 months ago by mjethani

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

comment:12 in reply to: ↑ 6 ; follow-up: Changed 19 months ago by 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.

comment:13 Changed 19 months ago by mjethani

  • Summary changed from [meta] Use the class keyword to [meta] Use the class keyword in JavaScript code

comment:14 in reply to: ↑ 12 Changed 19 months ago by 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

comment:15 follow-up: Changed 19 months ago by mjethani

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

comment:16 Changed 19 months ago by 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

Last edited 19 months ago by agiammarchi (previous) (diff)

comment:17 in reply to: ↑ 15 Changed 19 months ago by 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.

comment:18 Changed 19 months ago by 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.

comment:19 Changed 16 months ago by mjethani

  • Blocked By 6741 added
Note: See TracTickets for help on using tickets.