clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
491 stars 110 forks source link

Item.set doesn't work as advertised #174

Open aneilbaboo opened 5 years ago

aneilbaboo commented 5 years ago

I noticed two problems in Item.set.

  1. 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' }
  1. 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.

rchl commented 5 years ago

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.

aneilbaboo commented 5 years ago

That's a legitimate worry. However, the current behavior is a bug. So perhaps there's a path to fixing this:

  1. Add a property mergeWhenSettingObjectAttributes to the dynogels top level which sets an internal property
  2. 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. Use dynogels.mergeWhenSettingsObjectAttributes=false for overwrite behavior. Set to true for merge behavior."

    • else If value is set to falsey, use _.extend,
    • else value is truthy, so use _.merge.
  3. On next major version bump, switch the behavior to _.extend.
zolaemil commented 5 years ago

Unexpected as it is, it seems like the behaviour is by design, because Item.prototype.update uses Item.prototype.set

https://github.com/clarkie/dynogels/blob/6246c13ff8732d9508b854296970dfc592fef351/lib/item.js#L67