Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inherits replacement recommendation is not dev friendly #147

Open
Uzlopak opened this issue Aug 11, 2024 · 9 comments
Open

inherits replacement recommendation is not dev friendly #147

Uzlopak opened this issue Aug 11, 2024 · 9 comments

Comments

@Uzlopak
Copy link

Uzlopak commented Aug 11, 2024

The inherits package was added with #103 to solve #99 by @justinfagnani

I politely disagree with the whole remark, that you should just use "class syntax". Nobody with the right mind would imho rewrite everything to shave off 4 kb. Also just to claim that inherits is considered legacy, does not mean that it is deprecated. It just means that it will stay but will not be actively developed.

It would be closer to reality to recommend what inherits actually does by directly modifying the prototype of the Target with the Base

So instead of:

inherits(Target, Base)

you just do:

Target.prototype = Object.create(Base.prototype, {
   constructor: { value: Target, enumerable: false, writable: true, configurable: true }
 })

So you dont need to rewrite the functions to classes, just to remove inherits.

@Uzlopak Uzlopak changed the title inherits entry is not dev friendly inherits replacement recommendation is not dev friendly Aug 11, 2024
@justinfagnani
Copy link
Contributor

You don't have to rewrite your whole app just because there's a replacement recommendation.

JavaScript has a native way of easily inheriting now, and it's recommended by Node as a replacement for inherits:
Screenshot_20240811-220328

@Uzlopak
Copy link
Author

Uzlopak commented Aug 12, 2024

Again. Legacy does not mean deprecated. And there are cases where I purposefully did not use ES6 classes. The easiest replacement are the three lines i cited above.

@Lehoczky
Copy link
Contributor

The ES6 class syntax is easier to understand for most people in my experience, and it's so widely used, new JavaScript developers might not even know about util.inherits and they are not missing anything (IMO).

It's completely okay if you prefer the older syntax, but as already mentioned:

  • you don't have to rewrite your app, this is just a recommendation. It's up to you and the people you work with to decide whether you want to use util.inherits
  • the official Node.js docs recommend using the newer class syntax too

@Uzlopak
Copy link
Author

Uzlopak commented Aug 16, 2024

Sure. There are even old JS developers, who never learned the prototype way of inheritance, as they learned class inheritance from the beginning. I mean ES6 is ES2015, and that means that class inheritence is 9 years old.

Its not like I prefer any old syntax. I prefer classes, but there are cases where you want to use the prototype way.

Also i doubt that the official node.js docs is recommending anything. util.inherits is an old utility function, which would not be implemented nowadays if requested, as you can do it yourself with that three liner. Back in those days alot of stuff was added to nodejs, which is considered wrong nowadays. E.g. node:querystring. Following text of this module

`qs` can typically be replaced with platform-provided APIs. When you need more functionality, there are other alternatives.
implicitly refers to node:querystring

Fact is that the easiest way to replace the inherits plugin with the three lines I posted in my original post. And that should be documented.

@43081j
Copy link
Contributor

43081j commented Aug 18, 2024

i'd be open to moving inherits to the preferred manifest instead of the native manifest

that would then allow you to create a doc for it, which would explain both the class syntax and the Object.create syntax

@justinfagnani @Uzlopak what do you think? the linter will still pick it up, as i think this is a documentation issue rather than inherits not belonging in the manifests

@justinfagnani
Copy link
Contributor

@43081j what about both? The originator the library recommends using classes and extends. I think that's the best recommendation for modern code unless you have special requirements.

@justinfagnani
Copy link
Contributor

@Uzlopak

Also i doubt that the official node.js docs is recommending anything.

It says right in the docs that they recommend using class syntax.

@43081j
Copy link
Contributor

43081j commented Aug 19, 2024

do you mean both manifests? the current manifests are not meant to overlap since the linter will apply them as if they were "levels" of strictness

if we move it to the preferred manifest, it just means we can add a doc. it'll still be enabled in the linter and the CLI

@Uzlopak
Copy link
Author

Uzlopak commented Aug 19, 2024

Actually we need to read further in the docs:

Usage of util.inherits() is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support. Also note that the two styles are semantically incompatible.

Inherit the prototype methods from one constructor into another. The prototype of constructor will be set to a new object created from superConstructor.

This mainly adds some input validation on top of Object.setPrototypeOf(constructor.prototype, superConstructor.prototype). As an additional convenience, superConstructor will be accessible through the constructor.super_ property.

And semantically incompatible refers to:

nodejs/node#4179

So it is a little bit more complicated but probably Object.setPrototypeOf would be in many cases the replacement for inherits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants