{"action":"created","issue":{"url":"https://api.github.com/repos/andrewplummer/Sugar/issues/518","labels_url":"https://api.github.com/repos/andrewplummer/Sugar/issues/518/labels{/name}","comments_url":"https://api.github.com/repos/andrewplummer/Sugar/issues/518/comments","events_url":"https://api.github.com/repos/andrewplummer/Sugar/issues/518/events","html_url":"https://github.com/andrewplummer/Sugar/issues/518","id":111946468,"number":518,"title":"The future of Object.merge","user":{"login":"andrewplummer","id":74646,"avatar_url":"https://avatars.githubusercontent.com/u/74646?v=3","gravatar_id":"","url":"https://api.github.com/users/andrewplummer","html_url":"https://github.com/andrewplummer","followers_url":"https://api.github.com/users/andrewplummer/followers","following_url":"https://api.github.com/users/andrewplummer/following{/other_user}","gists_url":"https://api.github.com/users/andrewplummer/gists{/gist_id}","starred_url":"https://api.github.com/users/andrewplummer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andrewplummer/subscriptions","organizations_url":"https://api.github.com/users/andrewplummer/orgs","repos_url":"https://api.github.com/users/andrewplummer/repos","events_url":"https://api.github.com/users/andrewplummer/events{/privacy}","received_events_url":"https://api.github.com/users/andrewplummer/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/andrewplummer/Sugar/labels/Feature","name":"Feature","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/andrewplummer/Sugar/milestones/5","html_url":"https://github.com/andrewplummer/Sugar/milestones/2.0.0","labels_url":"https://api.github.com/repos/andrewplummer/Sugar/milestones/5/labels","id":1303610,"number":5,"title":"2.0.0","description":null,"creator":{"login":"andrewplummer","id":74646,"avatar_url":"https://avatars.githubusercontent.com/u/74646?v=3","gravatar_id":"","url":"https://api.github.com/users/andrewplummer","html_url":"https://github.com/andrewplummer","followers_url":"https://api.github.com/users/andrewplummer/followers","following_url":"https://api.github.com/users/andrewplummer/following{/other_user}","gists_url":"https://api.github.com/users/andrewplummer/gists{/gist_id}","starred_url":"https://api.github.com/users/andrewplummer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andrewplummer/subscriptions","organizations_url":"https://api.github.com/users/andrewplummer/orgs","repos_url":"https://api.github.com/users/andrewplummer/repos","events_url":"https://api.github.com/users/andrewplummer/events{/privacy}","received_events_url":"https://api.github.com/users/andrewplummer/received_events","type":"User","site_admin":false},"open_issues":57,"closed_issues":3,"state":"open","created_at":"2015-09-14T05:23:15Z","updated_at":"2015-11-25T08:17:18Z","due_on":null,"closed_at":null},"comments":16,"created_at":"2015-10-17T06:08:58Z","updated_at":"2015-11-25T08:39:10Z","closed_at":null,"body":"`Object.merge` has had a bit of a complicated history. Although it seems straightforward enough to merge properties from one object into another, there are a number of different ways to do it, and things that can only be handled during the merge operation itself. `Date.create` has had a similar problem, and beginning in 2.0.0 an options object will handle some of the smaller edge cases when creating dates. I'm not a huge fan of options objects as it's hard to remember the names of options -- having them all over Sugar would be horrible. However, once you have enough arguments it becomes a similar problem to remember their order, and it's also a hassle to write out `(false, false, true)` just to get to the option you want. `Object.merge` is becoming like this. There are 2 options now (\"deep\" and \"resolve\") and I would like to add 2 more. I'll list them out here (abbreviating here and there for space):\r\n\r\n\r\n### Options\r\n\r\n#### Deep Merging\r\n\r\nThis should be pretty obvious -- just recursively merging if properties themselves are objects. This is currently the 3rd argument and is `false` by default. Proposal is to stay false by default and become `deep` on the options object.\r\n\r\n```\r\nvar o1 = { name: { first: 'Darth' } };\r\nvar o2 = { name: { last: 'Vader' } };\r\nmerge(o1, o2); ->  { name: { last: 'Vader' } }\r\nmerge(o1, o2, true); ->  { name: { first: 'Darth', last: 'Vader' } }\r\nmerge(o1, o2, { deep: true }); // with options\r\n```\r\n\r\n#### Resolve\r\n\r\nWhen a conflict arises in a merge, which property should win? Right now the default is `true`, which means the source object's property wins -- in other words what you would expect from a standard merge. If this argument is `false` then the target's property wins, or what you would expect from a `defaults` method (side proposal: create a `defaults` method as a shortcut to this behavior). However this argument can also be passed a function:\r\n\r\n```\r\nvar o1 = { name: 'Darth'  };\r\nvar o2 = { name: 'Vader' };\r\nvar fn = function (key, targetVal, sourceVal) {\r\n  return targetVal + ' ' + sourceVal;\r\n}\r\nmerge(o1, o2); ->  { name: 'Vader' }\r\nmerge(o1, o2, false, false); ->  { name: 'Darth' }\r\nmerge(o1, o2, false, fn); ->  { name: 'Darth Vader' }\r\nmerge(o1, o2, { resolve: fn }); // with options\r\n```\r\n\r\nThe resolver function is intended to allow fine grained control over merge conflicts. However, right now it has some confusing behavior that can almost be considered buggy. First, deep merges will continue to step in to an object returned by the resolver function. This is confusing because the sprit of the resolver function is to allow the user to override standard merge behavior, so anything returned by the function should become the final value. Conversely, right now the resolver function will *always* use the return value of the function, even if that is undefined. I would like to change this so that the final value of the resolver function be used *if defined*. If undefined, then `Object.merge` will handle the merge in the default way. This will make it more intuitive for users to be able to step in and take control of merges for certain properties only, make it more aligned with the rest of `Object.merge` (as undefined properties are not merged normally), and also be in line with Lodash (not top priority, but nice). So (I'm squashing both behaviors here into one example):\r\n\r\nCurrent Behavior:\r\n\r\n```\r\nvar o1 = { user_count: 5, foo: 'bar', obj: { inner: 1 } };\r\nvar o2 = { user_count: 5, foo: 'baz', obj: { inner: 2 } };\r\nvar fn = function (key, targetVal, sourceVal) {\r\n  if (key === 'user_count') {\r\n    return targetVal + sourceVal;\r\n  }  else if (key === 'obj') {\r\n    // New object, something else entirely\r\n    return { a: 'b' }\r\n  }\r\n}\r\n// \"foo\" wasn't handled so it becomes undefined\r\n// \"obj\" was passed a new object, but since it is a deep merge the \r\n// result is traversed into. Since \"inner\" isn't handled it becomes undefined.\r\nmerge(o1, o2, true, fn); ->  { user_count: 10, foo: undefined, obj: { inner: undefined } }\r\n```\r\n\r\nProposed behavior:\r\n\r\n```\r\n// \"foo\" wasn't handled so it gets handled in the default way\r\n// \"obj\" was passed a new object, so this is respected and becomes the final value\r\nmerge(o1, o2, { deep: true, resolve: fn }); ->  { user_count: 10, foo: 'baz', obj: { a: 'b' } }\r\n```\r\n\r\n\r\n\r\n#### Non-enumerable Properties\r\n\r\n`Object.merge` currently only operates on enumerable properties. This should be correct for most use cases, however there are times when merging non-enumerable properties is desirable as well. Proposal is to add this as an option.\r\n\r\nCurrent behavior:\r\n\r\n```\r\nvar o1 = { foo: 'bar' };\r\nObject.defineProperty(obj, 'hiddenFoo', {\r\n  value: 'hiddenBar',\r\n  enumerable: false\r\n});\r\nvar o2 = merge({}, o1);\r\nconsole.log(o1.foo); // 'bar'\r\nconsole.log(o1.hiddenFoo; // 'hiddenBar'\r\nconsole.log(o2.foo); // 'bar'\r\nconsole.log(o2.hiddenFoo; // undefined\r\n```\r\n\r\nProposed behavior:\r\n\r\n```\r\nvar o2 = merge({}, o1, { hidden: true });\r\nconsole.log(o1.foo); // 'bar'\r\nconsole.log(o1.hiddenFoo; // 'hiddenBar'\r\nconsole.log(o2.foo); // 'bar'\r\nconsole.log(o2.hiddenFoo; // 'hiddenBar'\r\n```\r\n\r\nAs for the naming, I am proposing `hidden` because `non-enumerable` is difficult to write out, and `enumerable: false` does not mean the same thing. If there are other ideas for naming that would make this clearer, I'm open to them.\r\n\r\n\r\n#### Merging Property Descriptors\r\n\r\nCurrently, only properties with an associated \"value\" are accessed and merged. As with non-enumerable properties, I think that this is fine as a default. However, ES5 introduced accessors in the form of property descriptors, and I think it's time that Sugar finally supported this as an option.\r\n\r\n\r\nCurrent behavior:\r\n\r\n```\r\nvar data = { label: 'foo' };\r\nvar o1 = {\r\n  get label() {\r\n    return data.label;\r\n  },\r\n  set label(value) {\r\n    data.label = value;\r\n  }\r\n}\r\nvar o2 = merge({}, o1);\r\nconsole.log(o1.label, o2.label);  // foo foo \r\ndata.label = 'bar';\r\nconsole.log(o1.label, o2.label);  // bar foo (link created by setter is broken)\r\n```\r\n\r\nProposed behavior:\r\n\r\n```\r\nmerge({}, o2, { descriptor: true });  // produces a faithful merge of all accessors\r\n```\r\n\r\nAs for naming `descriptor` seems to make sense, although I'm not sure about singular vs. plural. Also open to other naming options as well. Note that property descriptors are not supported in IE8 and below, so this option will do nothing in those environments. However, accessors can't be used in the first place, so if you need to support <=IE8 you won't need this option anyway.\r\n\r\n\r\n### Summary\r\n\r\nTo summarize the proposals here:\r\n\r\n- Using an options object instead of listed arguments\r\n- Adding an option for non-enumerable properties (off by default)\r\n- Adding an option for property descriptors (off by default)\r\n- Changing the resolve function to not go deep (respect the final value if returned)\r\n- Changing the resolve function to do a default merge if the final value is undefined\r\n- Adding a method called `Object.defaults` that is a shortcut to `resolve: false`.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/andrewplummer/Sugar/issues/comments/159534901","html_url":"https://github.com/andrewplummer/Sugar/issues/518#issuecomment-159534901","issue_url":"https://api.github.com/repos/andrewplummer/Sugar/issues/518","id":159534901,"user":{"login":"andrewplummer","id":74646,"avatar_url":"https://avatars.githubusercontent.com/u/74646?v=3","gravatar_id":"","url":"https://api.github.com/users/andrewplummer","html_url":"https://github.com/andrewplummer","followers_url":"https://api.github.com/users/andrewplummer/followers","following_url":"https://api.github.com/users/andrewplummer/following{/other_user}","gists_url":"https://api.github.com/users/andrewplummer/gists{/gist_id}","starred_url":"https://api.github.com/users/andrewplummer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andrewplummer/subscriptions","organizations_url":"https://api.github.com/users/andrewplummer/orgs","repos_url":"https://api.github.com/users/andrewplummer/repos","events_url":"https://api.github.com/users/andrewplummer/events{/privacy}","received_events_url":"https://api.github.com/users/andrewplummer/received_events","type":"User","site_admin":false},"created_at":"2015-11-25T08:39:10Z","updated_at":"2015-11-25T08:39:10Z","body":"@Slayer95 Thanks for your input here. In the end I went with a slightly modified form of your idea. In the end the motivation was the fact that there was no way to bail out of merging a property in a resolve function as returning `undefined` meant \"merge as normal\". I feel that returning the global `Sugar` object is much more intuitive as it is telling the result to \"let Sugar handle the merge\"."}}