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

Fix util extend issue #2334

Closed
wants to merge 3 commits into from
Closed

Conversation

itkoren
Copy link

@itkoren itkoren commented Aug 9, 2015

When the "origin" object argument is null or not defined, _extend
throws exception.
Fix _extend to allow undefined "origin" object argument and return
the "add" object argument.

When the "origin" object argument is null or not defined, _extend
throws exception.
Fix _extend to allow undefined "origin" object argument and return
the "add" object argument.
@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Aug 9, 2015
@brendanashworth
Copy link
Contributor

Ref: nodejs/node-v0.x-archive#25672

@Fishrock123 Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 10, 2015
@ronkorving
Copy link
Contributor

Should we not be returning a copy of add? (ie: set origin = {}; and letting it run, instead of returning add)
Now you return an object that may or may not be the added object, the user could start mutating it and get unexpected results.

Imho, we should always return origin. If we have no origin, make one.

Fix _extend to allow undefined "origin" object argument and create
a new origin object to return, instead of returning "add" object
argument.
@itkoren
Copy link
Author

itkoren commented Aug 10, 2015

I totally agree @ronkorving. I've just pushed a fix for this suggested behavior, so the add object is immutable

@ronkorving
Copy link
Contributor

Thanks! 👍

@Fishrock123
Copy link
Contributor

Is the use case for this just cloning an object, or am I missing something?

@@ -715,6 +715,9 @@ exports._extend = function(origin, add) {
// Don't do anything if add isn't an object
if (add === null || typeof add !== 'object') return origin;

// Create new object for origin, if origin isn't an object
if (origin === null || typeof origin !== 'object') origin = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that the above condition uses || typeof add !== 'object', but I'd rather not see that added again down below. util._extend(36, {a: 42}) makes no sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @brendanashworth. util._extend(36, {a: 42}) is nonsensical and should probably blow up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@brendanashworth
Copy link
Contributor

I'm not sure that this is really pressing enough to be added. util._extend(object || {}, { ... }) seems like it would work fine for your usecase. Plus, util._extend is technically private (even though everyone uses it), so it isn't really designed to be used downstream. Thoughts on that?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

-1 on this. Not seeing the benefit (extending a null or undefined should blow up IMO). given the lack of activity, closing. Can reopen if anyone feel it is necessary.

@jasnell jasnell closed this Nov 16, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

FWIW, we also have Object.assign() now.

@ronkorving
Copy link
Contributor

Object.assign is quite slow unfortunately :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants