andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.53k stars 306 forks source link

Doesn't merge property by descriptor if property returns `undefined` #619

Open adque opened 6 years ago

adque commented 6 years ago

There is a bug when merging by descriptor. When merging a property that returns undefined, the property will not be merged. This is problematic since the value that property returns may later change.

Example:

var store = {};
var merged = Object.merge({}, { get value() { return store['x'] } }, { descriptor: true });
store['x'] = "testing...";

// logs undefined instead of "testing..." because the `value` property is not merged
// because it returns undefined at the time it is merged
console.log(merged.value);

var descriptor = Object.getOwnPropertyDescriptor(merged, 'value');
console.log(descriptor);  // logs undefined
andrewplummer commented 6 years ago

Hi, sorry for taking so long to respond to this. Unfortunately this is as the method is specced right now. Any properties that are undefined will not be merged. I agree that this is not ideal for situations like this and will have a look in the next major version about ways to resolve. I should also add that the merge method is entirely too complicated right now and doesn't accomplish the main use cases for the complexity required to use. Merging non-enumerable properties as well as merging property descriptors will likely be moved out of the main package into a plugin. At that point I'll revisit the method to make sure it's easier to use.

Thanks!