{"action":"created","issue":{"url":"https://api.github.com/repos/metarhia/Common/issues/1","repository_url":"https://api.github.com/repos/metarhia/Common","labels_url":"https://api.github.com/repos/metarhia/Common/issues/1/labels{/name}","comments_url":"https://api.github.com/repos/metarhia/Common/issues/1/comments","events_url":"https://api.github.com/repos/metarhia/Common/issues/1/events","html_url":"https://github.com/metarhia/Common/pull/1","id":212129264,"number":1,"title":"Make `clone` abstract again","user":{"login":"nechaido","id":17278974,"avatar_url":"https://avatars1.githubusercontent.com/u/17278974?v=3","gravatar_id":"","url":"https://api.github.com/users/nechaido","html_url":"https://github.com/nechaido","followers_url":"https://api.github.com/users/nechaido/followers","following_url":"https://api.github.com/users/nechaido/following{/other_user}","gists_url":"https://api.github.com/users/nechaido/gists{/gist_id}","starred_url":"https://api.github.com/users/nechaido/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nechaido/subscriptions","organizations_url":"https://api.github.com/users/nechaido/orgs","repos_url":"https://api.github.com/users/nechaido/repos","events_url":"https://api.github.com/users/nechaido/events{/privacy}","received_events_url":"https://api.github.com/users/nechaido/received_events","type":"User","site_admin":false},"labels":[{"id":553170537,"url":"https://api.github.com/repos/metarhia/Common/labels/enhancement","name":"enhancement","color":"84b6eb","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2017-03-06T13:56:42Z","updated_at":"2017-03-06T16:29:41Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/metarhia/Common/pulls/1","html_url":"https://github.com/metarhia/Common/pull/1","diff_url":"https://github.com/metarhia/Common/pull/1.diff","patch_url":"https://github.com/metarhia/Common/pull/1.patch"},"body":"* Rewrite `clone` so it properly copies a prototype.\r\n  This way we will need only one method instead of 3\r\n* Add optional clone depth parameter"},"comment":{"url":"https://api.github.com/repos/metarhia/Common/issues/comments/284450179","html_url":"https://github.com/metarhia/Common/pull/1#issuecomment-284450179","issue_url":"https://api.github.com/repos/metarhia/Common/issues/1","id":284450179,"user":{"login":"aqrln","id":4923335,"avatar_url":"https://avatars2.githubusercontent.com/u/4923335?v=3","gravatar_id":"","url":"https://api.github.com/users/aqrln","html_url":"https://github.com/aqrln","followers_url":"https://api.github.com/users/aqrln/followers","following_url":"https://api.github.com/users/aqrln/following{/other_user}","gists_url":"https://api.github.com/users/aqrln/gists{/gist_id}","starred_url":"https://api.github.com/users/aqrln/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aqrln/subscriptions","organizations_url":"https://api.github.com/users/aqrln/orgs","repos_url":"https://api.github.com/users/aqrln/repos","events_url":"https://api.github.com/users/aqrln/events{/privacy}","received_events_url":"https://api.github.com/users/aqrln/received_events","type":"User","site_admin":false},"created_at":"2017-03-06T16:29:40Z","updated_at":"2017-03-06T16:29:40Z","body":"While this implementation is much shorter, prettier and covers more usecases, I have admit that it is, maybe, too generic and we'll still have to have specialized functions too, since this PR brings a significant performance regression (at least, when cloning arrays).\r\n\r\nI though it could be improved by making arrays a special case, but it didn't give a very big boost.\r\n\r\n<details>\r\n  <summary>Patch to @nechaido's implementation</summary>\r\n  <p>\r\n\r\n```diff\r\ndiff --git a/lib/data.js b/lib/data.js\r\nindex 710617a..6cb1fbb 100644\r\n--- a/lib/data.js\r\n+++ b/lib/data.js\r\n@@ -32,6 +32,16 @@ api.common.clone = (\r\n     return obj;\r\n   }\r\n \r\n+  if (Array.isArray(obj)) {\r\n+    const size = obj.length;\r\n+    const array = new Array(size);\r\n+    let i;\r\n+    for (i = 0; i < size; i++) {\r\n+      array[i] = deepClone(obj[i], depth && depth - 1);\r\n+    }\r\n+    return array;\r\n+  }\r\n+\r\n   const clone = new obj.constructor();\r\n   Object.keys(obj).forEach(key => {\r\n     clone[key] = api.common.clone(obj[key], depth ? depth - 1 : undefined);\r\n```\r\n\r\n  </p>\r\n</details>\r\n<br>\r\n\r\nThough the original `cloneArray` function can be improved too.\r\n\r\n<details>\r\n  <summary>Patch to @tshemsedinov's implementation</summary>\r\n  <p>\r\n\r\n```diff\r\ndiff --git a/lib/data.js b/lib/data.js\r\nindex 5797fe9..23ea0dd 100644\r\n--- a/lib/data.js\r\n+++ b/lib/data.js\r\n@@ -50,18 +50,16 @@ function cloneObject(obj) {\r\n }\r\n \r\n function cloneArray(arr) {\r\n-  const result = [];\r\n   const len = arr.length;\r\n+  const result = new Array(len);\r\n   let i, val, type;\r\n   for (i = 0; i < len; i++) {\r\n     val = arr[i];\r\n     type = typeof(val);\r\n     if (type === 'object') {\r\n-      result.push(\r\n-        Array.isArray(val) ? cloneArray(val) : cloneObject(val)\r\n-      );\r\n+      result[i] = Array.isArray(val) ? cloneArray(val) : cloneObject(val);\r\n     } else {\r\n-      result.push(val);\r\n+      result[i] = val;\r\n     }\r\n   }\r\n   return result;\r\n```\r\n\r\n  </p>\r\n</details>\r\n<br>\r\n\r\nBenchmark results:\r\n\r\n```\r\ntshemsedinov's cloneArray x 3,317,870 ops/sec ±1.25% (85 runs sampled)\r\nnechaido's clone x 137,082 ops/sec ±0.73% (90 runs sampled)\r\npatched tshemsedinov's cloneArray x 3,516,861 ops/sec ±1.31% (91 runs sampled)\r\npatched nechaido's clone x 162,357 ops/sec ±0.88% (91 runs sampled)\r\nFastest is patched tshemsedinov's cloneArray\r\n```\r\n\r\n[Benchmark source code](https://gist.github.com/aqrln/586be14f0132e4e449f375536582e06f)"}}
{"actor":{"display_login":"aqrln"}}