Regression in 2.0.0
sindresorhus opened this issue ยท 11 comments
This commit b55d7b6 seems to cause problem in XO.
I've added a failing test: efa61ab
It seems at deep level it no longer creates a new object but reuses the existing one.
@schnittstabil Could you take a look?
That happens, because we do not recurse into objects in v2, which may be configurable by introducing options (as discussed in #4):
const fixture = {
foo: {
bar: false
}
};
const run = x => {
const opts = fn({}, fixture);
//=> opts.foo === fixture.foo
if (x === true) {
opts.foo.bar = true;
//=> opts.foo.bar === fixture.foo.bar === true
}
return opts.foo.bar;
};
t.true(run(true));
//=> fixture.foo.bar === true
t.false(run());
we do not recurse into objects in v2
I must have missed that in the original discussion... But what's the point then of deep assign, if it doesn't assign deeply? It should really be the default behavior.
deep-assign recurses, if there is a corresponding target property:
function User(name) {
this.name = name;
}
var target = {
user: new User('Alice')
};
var source = {
user: new User('Bob')
};
// target.user exists, so deep-assign steps into target.user and source.user:
deepAssign(target, source);
//=> target.user.name === source.user.name === 'Bob'
// but: target.user !== source.user
However, deep-assign simply sets the target property, if there is no corresponding property:
var target = {
};
var source = {
user: new User('Bob')
};
deepAssign(target, source);
//=> target.user === source.user
The reason for this is that we can not clone arbitrary source objects in a sensible way, e.g. objects with a prototype chain. (Also functions are objects too.)
deep-assign is used as a option-merge-tool in XO. I think that will sooner or later be in conflict with deep-assigns main intention:
Recursive Object.assign()
An example:
const DEFAULT_CONFIG = {
filter: () => true,
};
const opts = {
filter: n => n % 2,
};
var config = deepAssign(DEFAULT_CONFIG, opts);
//=> config.filter === DEFAULT_CONFIG.filter
That's because all properties of opts.filter
are copied to DEFAULT_CONFIG.filter
๐
I would suggest to create a new project for this, with defaults like array-slicing, plain-object-cloning, function-overwriting etc. I would be happy to help.
@sindresorhus I started working on a option-merge-tool
what's the problem with this? https://www.npmjs.com/package/deep-extend
@ruffle1986 For most use cases: nothing - But I want some other semantics, e.g. the following doesn't seems reasonable to me:
deepExtend({}, {p: Promise.resolve(42)})
//=> { p: {} }
EDIT: But most importantly deep-extends
copies buffers - I think that is not very sensible for merging options.
I've just published merge-options, which is based on is-plain-obj.
@sindresorhus @ruffle1986 @coderhaoxin @sugarshin: reviews, comments and feature request are welcome
I was trying to use deep-assign to merge a user provided options object with my defaults and encountered an issue similar to this #6 (comment) (function from defaults wasn't replaced by the function from options). Ended up, trying out merge-options and it worked well for me.
I think the readme should contain some examples of the edge cases and a link to merge-options in the related section.
Yeah, I got caught by this also.
Probably should have been more vigilant but a quick mention of the issue might be useful in the Readme since it arguably deviates from the description of "Recursive Object.assign()".
This module is now deprecated: b332062