util: deprecate util._extend

[rmg]

Mark util._extend as deprecated and replace all internal usage with the
language provided Object.assign.

util._extend is a long standing member of the undocumented and
unofficial public API. It has never been removed because, quite frankly,
it is ridiculously useful and is used extensively throughout node core
itself.

With the addition of support for Object.assign in node, and the use of
other ES6isms, there is now an alternative to maintaining util._extend
and adding it to the API docs.

/cc @sam-github @jasnell

[cjihrig]

I think Object.assign() can handle this without the || {}

[rmg]

Good point. With one exception, this was mainly a find/replace. I'll go through it once more to clean up this kind of usage.

[evanlucas]

Although it is not documented, I've seen this used quite a bit in the wild.

[cjihrig]

LGTM if the CI is happy.

I'm in favor of this change even without the deprecation of _extend(). If we do the deprecation, you might want to add the suggested alternative to the deprecation message.

[ChALkeR]

It's used by request, glob, pm2, node-sass, spdy, http2, and a lot of other modules.

I'm +1 to this change, but this requires a deprecation imo.

Update: request and glob are fixed now.

[mscdex]

+1 to adding a suggested alternative (e.g. Object.assign()) in the deprecation message.

[rmg]

@ChALkeR

I'm +1 to this change, but this requires a deprecation imo.

That's what this PR does. Actual removal would be a while down the road.

[rmg]

@mscdex I thought about which alternative to recommend but couldn't think of one that didn't involve picking a favourite or relying on Node >= 0.12. I guess Object.assign() makes sense from an "encouraging forward movement" perspective.

[ChALkeR]

@rmg I noticed that, but a comment just above mine mentioned removing this without deprecation, that was why I expressed my opinion about that.

[rmg]

@ChALkeR ah, I see now. 👍

[cjihrig]

I noticed that, but a comment just above mine mentioned removing this without deprecation

My comment didn't mean to remove without deprecation. It meant that I'm in favor of this change even if we don't touch util._extends().

[mscdex]

@rmg This change would be going into a version of node post-v0.12, so I wouldn't be concerned. v0.12 users would not see this deprecation message.

[mscdex]

CI: https://ci.nodejs.org/job/node-test-pull-request/1172/

[Qard]

Not sure I like the idea of suggesting switching to Object.assign() at this point. Only more recent node versions support it. Module writers probably shouldn't be using it yet if they want their modules to work on those older node versions.

[mscdex]

@Qard The way I interpret messages like this is that they only apply for that particular version because the only way you're seeing it is if you're already using a newer node version. To me this is different than the API docs where people may bookmark the /docs/latest/api and use that as a reference, no matter what node version they are actually programming against.

shrug

[silverwind]

A well known alternative would be the util-extend package. By the way, this doesn't strictly need to be semver-major, deprecation can be done in a minor, just the removal has to be major.

[Qard]

@mscdex Yes, but a module author might try to use util._extend, encounter the deprecation message, and think it's safe to just switch the Object.assign, which is not necessarily the case.

I'm not firmly against the message, I just wanted to bring attention to this scenario to think about.

[silverwind]

I think we should be able to expect module authors to know that Object.assign is an ES6 addition.

LGTM

[mscdex]

LGTM

[ChALkeR]

@silverwind Given how many of popular modules use util._extend directly or indirectly, this should probably be a semver-major.

@rmg Did you test npm? Atm it bundles its deps, and depends on request and glob, both of which utilize util._extend. Will this PR make bundled npm emit util._extend deprecation warnings under some use-cases?

[sam-github]

@rmg I think its worth breaking into two PRs. Internally changing to Object.assign() would be a patch.

Deprecating util._extend() would be semver-major. This is going to cause a lot of churn, including pretty much all my modules, but that's OK. I'm on record favouring a smaller core Node.js, I'll suck it up.

@Qard Many deprecations, such as exists in favour of access, has a chance of not working on older Node.js. Its a bit touch keeping compat with 0.10, takes care, and the fact that our docs don't mention at which node version a feature was introduced or modified is not helpful.

[cjihrig]

Splitting it into two commits in this PR would work too.

[JacksonTian]

The Object.assign slower than _extend 33%.

[rmg]

I thought about splitting it into 2 commits but couldn't decide at the time which order made more sense so I punted and combined them.

I guess the deprecation makes more sense after removing all usage. I'll split it that way.

[noscripter]

-1 for deprecating this undocumented yet so useful util function

[rmg]

The ES6 part is actually coincidental, the actual goal is to move toward a more clearly defined API.

It was a coin toss between submitting a PR that deprecates it or submitting a PR that documents it. Since I'm not particularly fond of util._extend's limitations (1 source only), I skipped the literal coin toss and did the one I thought would be more fun.

[evanlucas]

I'm -1 on deprecating it as well.

[jasnell]

Hmmm, ok, so at this point we have several LGTM's but also several -1's. I'm going to recommend that we drop this onto the @nodejs/ctc agenda for a quick discussion.

[rmg]

@jasnell good idea.

I'm more than willing to replace this PR with a different PR that adds docs for util._extend if the -1's are meant as implicit +1's for that approach.

[ChALkeR]

I would vote -1 for adding something like util._extend to the documented public API at this point — in the long term, something like Object.assign should be used instead.

[cjihrig]

We could just replace our internal uses of util._extend() and not do anything else. We can revisit the deprecation after Object.assign() becomes more widely adopted.

[domenic]

Differences between util._extend and Object.assign, in descending order of interestingness:

• Object.assign takes more than one argument. This can be important since e.g. arr.reduce(util._extend, ...) or arr.forEach(util._extend.bind(undefined, obj)) will work differently than Object.assign.
• Object.assign will convert its first argument to an object, e.g. if it is a primitive. Additionally, this will cause it to always blow up if the first argument is undefined or null; util._extend will only blow up on those if any actual properties are to be copied.
• util._extend appears to blow up on undefined second argument. Object.keys ignores any undefined arguments.
• util._extend assigns keys backward, starting at the last one returned from Object.keys, whereas Object.assign goes forward. This only matters if a setter on the target object observes the target object's state.
• util._extend uses Object.keys and keys.length public APIs, whereas Object.assign goes directly to the internal keys. This matters if someone has done Object.keys = ... or other crazy monkeypatches.

[ChALkeR]

@domenic Does that include #4593 (comment)?

[domenic]

@ChALkeR good catch, will update!

[benjamingr]

I just read the consensus.

I think we can start by replacing all internal usage of _extend to assign.

[rvagg]

CTC resolution regarding deprecation of util._extends, for now was this:

Document deprecation, but don’t warn on use.

Someone needs to do that, however. It's not even in the docs so the idea was to doc it and say that it's deprecated and shouldn't be used, "use Object.extend() instead", or something like that.

It's in too heavy usage. See 2016-01-20 minutes that are going in via PR shortly, audio also available.

[benjamingr]

@rvagg made a quick PR.

[silverwind]

I think we can start by replacing all internal usage of _extend to assign.

Didn't someone mention that the perf is worse with Object.assign?

[rmg]

Closing in favour of #4903 - I'm happy just to have kicked off the conversation :-)

Thanks for picking up the slack while I was absent, @benjamingr!