chore: remove inherits by migrating to ES6 class syntax

[yassernasc]

Refactor to remove the usage of util.inherits in favor of the recommended approach (https://nodejs.org/docs/v22.11.0/api/util.html#utilinheritsconstructor-superconstructor).

Unfortunately, the git diff got a bit confused because of the indentation level change. The PR is mainly an adaptation from one syntax to another, the more opinionated points are:

• This function is needed for not adding any breaking change, we still can create instances with and without the new keyword.

function Boot (...args) {
  return new _Boot(...args)
}

• Now at a class always being initialized with new, this piece of code was removed:

if (!new.target) {
   return new Boot(server, opts, done)
}

---

The motivation of this PR is interoperability with an alternative JS runtime. I am trying to execute avvio along with an EventEmitter implementation that is ES6 class-based.

---

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[jsumners]

Why? Is there a discussion in an issue I may have missed where it was decided this is desired?

[yassernasc]

Oh, there's no previous discussion about it, although a similar attempt was tried a while ago: #236. It's more a fix on my end that I hope to be a contribution with mutual benefit.

[jsumners]

What is it fixing?

[gurgunday]

We don't do refactors like these when they aren't desperately needed, it always risks introducing bugs without adding much

[mcollina]

You can use the following two to remove the dependency on util.inherits:

https://github.com/nodejs/node/blob/be5a500ae39baba2747be1b94972ef8d224fcb49/lib/internal/streams/readable.js#L95-L96

[jsumners]

You can use the following two to remove the dependency on util.inherits:

nodejs/node@be5a500/lib/internal/streams/readable.js#L95-L96

For reference, the linked example is a Node.js specific implementation of the JavaScript native way to define the prototype chain. Refer to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain#building_longer_inheritance_chains.

[yassernasc]

The suggested approach to remove the dependency still won't resolve the issue (from another JavaScript runtime, which is Node.js-like but not Node.js), the situation is: Using setPrototypeOf all the methods/props from the base prototype are copied but its constructor isn't executed, that lack of proper initialization leads our EventEmitter implementation (https://github.com/holepunchto/bare-events/blob/cef9d7271e4a5f98431a0883ee606d965ef7d0cb/index.js#L98) to a broken state. Example:

class Foo {
  constructor () {
    this.name = 'foo'
  }
}

function Bar() {
  this.version = '0.0.0'
  
  if (!new.target) return new Bar()
}

Object.setPrototypeOf(Bar.prototype, Foo.prototype)
Object.setPrototypeOf(Bar, Foo)

console.log(Bar().name) // <- undefined

That's true that we (maintainers from that alternative JavaScript runtime) could make workarounds to support avvio current source code but we have reasons to not do so: 1) Changing an object shape breaks optimizations from V8 (https://v8.dev/docs/hidden-classes) and the EventEmitter is a very hot point from our projects; 2) We try to avoid as possible supporting legacy syntaxes (I know that's a subjective point and I don't have the intention to enter on it).

The comment from @gurgunday is very  straightforward and reasonable, with everything well clarified, I think we can close the PR if I am not missing something.
Thank you for your time!

[jsumners]

The suggested approach to remove the dependency still won't resolve the issue

What is that issue? I haven't seen a description or minimal reproduction of a bug.

[yassernasc]

I've updated the comment, it's hard to give the right amount of context, ironically now I see that probably I gave too much, haha. Sorry.

[jsumners]

This code is spec compliant JavaScript. I think it is a runtime bug if you are having issues with it on a specific runtime.

[yassernasc]

That's true, the current avvio implementation is a perfect spec-wise javascript code, but not so spec-wise with the documentation from the current Node.js LTS version, there we see EventEmitter being defined as a class, also all examples initialize EventEmitters with the new keyword, and extends them with the extends keyword (https://nodejs.org/docs/latest-v22.x/api/events.html#class-eventemitter). Well, It's conceptually wrong to extend something would invoking the constructor of the extended class, so JavaScript evolved to resolve it at a syntax level.

I realized that I should have created an issue first before actually writing the code, it won't happen again. Lastly, there's no strong feeling about this, any decision will be smoothly respected. :)

[mcollina]

I would actually recommend to update your runtime to lift the EventEmitter implementation from Node.js: https://github.com/nodejs/node/blob/main/lib/events.js

You'll have far less compatibility issues.

[jsumners]

there we see EventEmitter being defined as a class, also all examples initialize EventEmitters with the new keyword, and extends them with the extends keyword (nodejs.org/docs/latest-v22.x/api/events.html#class-eventemitter)

I don't understand how this affects the standard code we have in this repo. The Node.js EventEmitter is not defined with the class syntax -- https://github.com/nodejs/node/blob/be5a500ae39baba2747be1b94972ef8d224fcb49/lib/events.js#L203-L210

I am closing this issue because it does not benefit the project and does not seem to fix a bug. All evidence points to a bug elsewhere. If you're trying to implement a Node.js compatible event emitter, I suggest reviewing the above linked code. An example in the documentation is not a specification for the implementation. If that's not what you're trying to do, I suggest reviewing how your custom runtime handles prototype chains and the class sugar.

[yassernasc]

Not a problem, life certainly finds its path, and again, thanks for the advice and time of each of you.

[mcollina]

Just for clarity fro anybody that stumble on this in the future, I have done a simple benchmark of classes vs functions, and apparently, classes are measurably slower at instantiation https://github.com/mcollina/class-vs-function.

[kasperisager]

@mcollina That's not the case on my end:

$ node -v
v23.2.0
$ node test.js
╔══════════════╤══════════╤═══════════════════╤═══════════╗
║ Slower tests │  Samples │            Result │ Tolerance ║
╟──────────────┼──────────┼───────────────────┼───────────╢
║ functions    │ 10000000 │ 5880756.30 op/sec │  ± 0.13 % ║
╟──────────────┼──────────┼───────────────────┼───────────╢
║ Fastest test │  Samples │            Result │ Tolerance ║
╟──────────────┼──────────┼───────────────────┼───────────╢
║ classes      │ 10000000 │ 9350010.99 op/sec │  ± 0.16 % ║
╚══════════════╧══════════╧═══════════════════╧═══════════╝