Item.set doesn't work as advertised
Opened this issue · 3 comments
I noticed two problems in Item.set
.
- The two-argument usage advertised in the docs here:
tweet.set('content', 'This is the content')
doesn't work.
Inspecting tweet.attrs
reveals the value to be
{ '0': 'c',
'1': 'o',
'2': 'n',
'3': 't',
'4': 'e',
'5': 'n',
'6': 't' }
- Object values are merged
draft.set({foo: {a: 1}});
draft.get('foo'); // => {'a': 1} as expected
draft.set({foo: {b: 2}});
draft.get('foo'); // => {'a': 1, 'b': 2} !!!
// expecting {'b': 2}
The problem is that _.merge
is used, which causes recursive merging.
_.extend
should be used instead.
Changing from merge to extend would probably be backwards-incompatible change and potentially even dangerous if there are existing users that depend on merging strategy right now, no? It should probably be configurable either from the set
call or globally but default to current behavior.
That's a legitimate worry. However, the current behavior is a bug. So perhaps there's a path to fixing this:
- Add a property
mergeWhenSettingObjectAttributes
to the dynogels top level which sets an internal property - Add a conditional warning:
- if this value is undefined, and use the default
_.merge
behavior:
"Warning: Please explicitly set mergeWhenSettingObjectAttributes. see
https:\\github.com\clarkie\dynogels\issues\174
for details. Current default behavior of Item.set, when value is an object, is to merge the new object with the existing object. In a future major release, this will change to overwrite. Usedynogels.mergeWhenSettingsObjectAttributes=false
for overwrite behavior. Set totrue
for merge behavior."- else If value is set to falsey, use
_.extend
, - else value is truthy, so use
_.merge
.
- if this value is undefined, and use the default
- On next major version bump, switch the behavior to
_.extend
.