This repository has been archived by the owner. It is now read-only.

Adding support for util.extend(origin, [...]); #4834

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@jimbojw

jimbojw commented Feb 22, 2013

The extend function is a staple of modern JavaScript
development. Yes, it's easy to implement, and everyone does it.
But functionality that everyone implements is exactly the kind
of thing that belongs in the core.

The extend function is so useful, in fact, that node already has
one, it just happens to be called _extend to make it secret.
Let's make it public and save everyone the burden of re-
inventing this particular wheel every time we make something.

Adding support for util.extend(origin, [...]);
The extend function is a staple of modern JavaScript
development. Yes, it's easy to implement, and everyone does it.
But functionality that everyone implements is exactly the kind
of thing that belongs in the core.

The extend function is so useful, in fact, that node already has
one, it just happens to be called _extend to make it secret.
Let's make it public and save everyone the burden of re-
inventing this particular wheel every time we make something.
@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 22, 2013

But functionality that everyone implements is exactly the kind of thing that belongs in the core.

I was with you until 'the core'.

node.js core is intentionally bare-bones. Everything that's not absolutely required should go into a npm module.

@bnoordhuis bnoordhuis closed this Feb 22, 2013

@xavi-

This comment has been minimized.

xavi- commented Feb 22, 2013

+1

Several of my projects have a lo-dash dependency simply because I didn't feel like re-writing .extends. It'd be nice to have it in core.

Also, util already has .inherits and .format which, IMO lies in the same philosophical arena as the .extends -- all could theoretically be userland libraries, but they make dev much easier when they're immediately available.

@jimbojw

This comment has been minimized.

jimbojw commented Feb 23, 2013

node.js core is intentionally bare-bones. Everything that's not absolutely required should go into a npm module.

I'd contend that extend is absolutely required, as evidenced by the fact that the util module has a _extend() function which is used liberally elsewhere in the node codebase.

Either it really is an absolutely necessary function, or each node core module should implement its own local extend() function.

@jimbojw

This comment has been minimized.

jimbojw commented Feb 23, 2013

Further, by that logic, most of util itself shouldn't be there. Prototypal inheritance isn't all that complicated, everyone could do it themselves, but there's a util.inherits() nonetheless.

@isaacs

This comment has been minimized.

isaacs commented Feb 23, 2013

Further, by that logic...

Luckily, this is a software program, and not a syllogism. We are perfectly comfortable with hypocrisy when it serves our purposes.

Most of util is used throughout node core internally. Yeah, sure, it'd be better to not need such generic reusables, and if any of them are not being used, we would have removed them by now.

util.inherits() is only like 10 lines or so. But it's 10 lines we'd repeat a lot, in subtly different ways, so we make it a reusable utility. Same with _extend. We made it initially internal, to make it even more clear that this is only here because node actually uses it.

You can go ahead and use util._extends() if you want. It's not going anywhere any time soon. You can even wrap it exactly like you have, and publish an npm module for it. Then you don't have to care anyone else's irregular logic, that's the beauty part :)

@jimbojw

This comment has been minimized.

jimbojw commented Feb 23, 2013

Luckily, this is a software program, and not a syllogism. We are perfectly comfortable with hypocrisy when it serves our purposes.

I'm glad you acknowledge the ridiculousness of the situation. :)

You can go ahead and use util._extends() if you want. It's not going anywhere any time soon.

Great! I'll write up a few paragraphs in the doc on how it works so that others can discover it and make use of it.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 23, 2013

Are you trolling? I can't tell.

It's an internal function. No one is stopping you from using it but we won't document it.

@rodrigoalvesvieira

This comment has been minimized.

rodrigoalvesvieira commented Feb 23, 2013

Whoa, now it is new to me. Internal functions aren't supposed to be documented?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 23, 2013

Of course. Documenting a function means promoting it to public API status - which util._extend() definitely isn't.

@jimbojw

This comment has been minimized.

jimbojw commented Feb 23, 2013

Are you trolling? I can't tell.

I was serious about documenting the _extend() function. Isaacs said to go ahead and use it, it's not going anywhere. It's a stable function upon which the node core clearly relies and it hasn't changed in a long time.

Node should have a non-external-code answer to the extend problem other than "do it yourself". And it does! In the form of this currently undocumented, hidden but publicly available, stable core feature.

So yes, if the advice is to go ahead and use it, then it should be documented and discoverable. I'd also be happy with a line in util that produces an unprefixed alias if we're uncomfortable promoting a prefixed function (e.g. exports.extend = exports._extend = ...)

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 23, 2013

Okay, apologies.

So yes, if the advice is to go ahead and use it, then it should be documented and discoverable.

Then please don't use it. It does exactly what we core hackers need but no more. There's probably a ton of edge cases where it won't work as expected (e.g. objects from different VM contexts).

As a general observation, we try to reduce the API - not expand it.

@jimbojw

This comment has been minimized.

jimbojw commented Feb 23, 2013

There's probably a ton of edge cases where it won't work as expected

I remember reading discussion of this. I believe the problem stems from the name "extend" which has been co-opted by numerous libraries with differing functionality. Some people will want deep copies, other people will want ES5 property descriptors or hasOwnProperty checks, etc.

If the name is the problem, then what about a new name? util.copyProps maybe? I'm more interested in making this simple bit of functionality public than exactly what the name is.

As a general observation, we try to reduce the API - not expand it.

Acknowledged - it's a principle I agree with, and one that many successful projects employ.

In this particular case, not having a core property copying function leads to a lot of wasteful duplication of effort. With almost any feature, someone can make the correct observation that technically it doesn't need to go in the core.

For instance, one could implement one's own HTTP parsing and just use TCP sockets directly. Having a fast, low-profile way of handling HTTP requests is so useful that it lives in the core offering.

Copying properties from one object to another belongs in this category. People who have edge cases can go to a more full-featured third party library, just like people who want a full-featured URL routing solution.

Some form of basic property copying belongs in the core, it's already there, it's stable, let's expose it.

@rodrigoalvesvieira

This comment has been minimized.

rodrigoalvesvieira commented Feb 23, 2013

@bnoordhuis thanks for the clarification.

@isaacs

This comment has been minimized.

@xavi-

This comment has been minimized.

xavi- commented Feb 23, 2013

No, not really. I don't think anyone was having trouble finding an implementation of extends. In fact, one of the recurring complaints was that too many people are reinventing this wheel. So, adding yet another implementation to npm hardly improves the situation or helps anyone.

@luckydrq

This comment has been minimized.

luckydrq commented Sep 24, 2014

ah, it seems an old story to me. Still i'm +1 with @jimbojw because in most of my node projects i have to require underscore or Lodash kind of thing just for javascript object mixin.

@sam-github

This comment has been minimized.

Member

sam-github commented Sep 24, 2014

Just use util._extend(), its not going away or changing.

@xavi-

This comment has been minimized.

xavi- commented Sep 24, 2014

From @bnoordhuis earlier in the thread.

Then please don't use it [util._extend]. It does exactly what we core hackers need but no more. There's probably a ton of edge cases where it won't work as expected (e.g. objects from different VM contexts).

@jimbojw

This comment has been minimized.

jimbojw commented Sep 24, 2014

var newObj = JSON.parse(JSON.stringify(originalObj));
@osher

This comment has been minimized.

osher commented Mar 3, 2016

@jimbojw that's an abuse of unecessary string manipulation. the bigger your object the more inefficient it is. I never recommend it.

@njam njam referenced this pull request Apr 18, 2016

Merged

Initial code #1

1 of 2 tasks complete
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.