{"action":"created","issue":{"url":"https://api.github.com/repos/omniscientjs/omniscient/issues/76","labels_url":"https://api.github.com/repos/omniscientjs/omniscient/issues/76/labels{/name}","comments_url":"https://api.github.com/repos/omniscientjs/omniscient/issues/76/comments","events_url":"https://api.github.com/repos/omniscientjs/omniscient/issues/76/events","html_url":"https://github.com/omniscientjs/omniscient/issues/76","id":59322029,"number":76,"title":"Simplify `shouldComponentUpdate`","user":{"login":"Gozala","id":21236,"avatar_url":"https://avatars.githubusercontent.com/u/21236?v=3","gravatar_id":"","url":"https://api.github.com/users/Gozala","html_url":"https://github.com/Gozala","followers_url":"https://api.github.com/users/Gozala/followers","following_url":"https://api.github.com/users/Gozala/following{/other_user}","gists_url":"https://api.github.com/users/Gozala/gists{/gist_id}","starred_url":"https://api.github.com/users/Gozala/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Gozala/subscriptions","organizations_url":"https://api.github.com/users/Gozala/orgs","repos_url":"https://api.github.com/users/Gozala/repos","events_url":"https://api.github.com/users/Gozala/events{/privacy}","received_events_url":"https://api.github.com/users/Gozala/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":28,"created_at":"2015-02-28T03:01:01Z","updated_at":"2015-02-28T10:27:34Z","closed_at":null,"body":"While I've being working on #64 I have being refactoring `shouldComponentUpdate` in order to have a predicate that could be used to compare current & past input of the function.\r\n\r\nCurrent implementation seems little over-engineered, but likely I don't have enough history of the code base to asses all constraints it needs to deal with, so I though I'll start a thread here to propose simplifications and learn why they can or can not be made. I'll start with few facts:\r\n\r\n1. Data structures from Immutable.js do have comparison operator `.equal` built into them & there for they could be compared simply by `expect.equals(actual)`. Also note that two immutable data structures may not satisfy this check https://github.com/omniscientjs/omniscient/blob/master/shouldupdate.js#L182 while it would `expect.equals(actual)` here is an example:\r\n\r\n  ```js\r\n  Immutable.List.of(1, 2) == Immutable.List.of(1, 2) // => false\r\n  Immutable.List.of(1, 2).equals(Immutable.List.of(1, 2)) // => true\r\n  ```\r\n\r\n2. Cursors at (least from Immutable.js) also do implement `.equal` operator on them & similar to comparing immutable data structures it will return `false` in cases where use of `.equal` would have returned `true`.\r\n\r\n3. The only other thing that `sholudComponentUpdate` needs does is ignores special properties like `statics` and `children` (I was actually wondering why it is ignoring children). Which got me thinking that\r\nthe whole thing could be simplified & speeded up greatly if we did something slightly different for statics, which I'm gonna cover later.\r\n\r\n\r\nGiven all that, I would imagine `shouldComponentUpdate` could be simplified to:\r\n\r\n```js\r\n\"use strict\";\r\n\r\nvar isEqual = require('lodash.isequal');\r\n\r\nfunction isComparable(value) {\r\n  return value && typeof(value.equals) === 'function';\r\n}\r\n\r\nfunction isEqualComparable(expect, actual) {\r\n  return isComparable(expect) ? expect.equals(actual) :\r\n         isComparable(actual) ? false :\r\n         void 0;\r\n}\r\n\r\n\r\nfunction shouldComponentUpdate(nextProps, nextState) {\r\n  return !isEqual(nextProps, this.props, isEqualComparable) ||\r\n         !isEqual(nextState, this.state, isEqualComparable);\r\n}\r\n```\r\n\r\nNow you may thing that will not cover statics, but I think that would be easy to fix by statics internally into something like:\r\n\r\n```js\r\nfunction AlwaysEqual(value) {\r\n   this.value = value\r\n}\r\nAlwaysEqual.prototype.equals = function(value) {\r\n   return true;\r\n}\r\n```\r\n\r\nAnd instead of doing this https://github.com/omniscientjs/omniscient/blob/master/component.js#L197-L201 we could just do:\r\n\r\n```js\r\n// If statics is a node (due to it being optional)\r\n// don't attach the node to the statics prop\r\nif (!!statics && !props.statics && !_isNode(statics)) {\r\n   _props[component.shouldNotUpdate] = AlwaysEqual(statics);\r\n} else {\r\n  _props[component.shouldNotUpdate] = AlwaysEqual(statics);\r\n}\r\n```\r\n\r\nWhich could be then unboxed before invoking a render, which basically will change this line\r\nhttps://github.com/omniscientjs/omniscient/blob/master/component.js#L145 to:\r\n\r\n```js\r\nreturn options.render.call(this, input, this.props[component.shouldNotUpdate]);\r\n```\r\nThis not only would improve performance of `shouldComponentUpdate` & simplify it but would also open door for great new opportunities, here is a short list:\r\n\r\n1. Will lead a natural path for statics validation proposed in #75, only thing that will be required is to make \r\nupdate boxing type form `AlwaysEqual` to something smarter that will return `false` in case transparent rewiring could not be done.\r\n2. In addition it would solve problem again stated in #75 of how do we let third parties rewire their custom communication channels. With this it's going to be easy the would just need to have their own boxing type that will have to implement `.equals` to make a call weather rewiring is possible and `[component.rebase]` to redirect communication to a new target. I think it will look very elegant here are few examples:\r\n\r\n  ```js\r\n  View({cursor, output: Output({remove, change}) })\r\n  ```\r\n\r\n  Note that key of the prop no longer matters it can be whatever, in this case `Output` will be something along these lines:\r\n\r\n  ```js\r\n  function Output(channels) {\r\n    if (this instanceof Output) {\r\n     if (channels.pipe) {\r\n       throw TypeError('pipe is reserved key and can not be used please use different name')\r\n     }\r\n     if (channels.equals) {\r\n       throw TypeError('equals is reserved key and can not be used please use different name')\r\n     }\r\n    \r\n     Object.assign(this, channels);\r\n    } else {\r\n      return new Output(channels);\r\n    }\r\n  }\r\n  Output.prototype.equals = function(other) {\r\n     // If all are channels we can re direct to them.\r\n     return lodash.isEqual(this, other, function(a, b) {\r\n        return isChannel(b);\r\n     });\r\n  };\r\n  Output.prototype[component.rebase] = function(target) {\r\n    Object.keys(target).forEach(function() {\r\n      this[key].pipe(target[key]);\r\n    }, this);\r\n  };\r\n  ```\r\n  In fact this can be taken even a step forward if channel, signal, event emitter or else would opt-in and implement these two methods they would not even need to be boxed and could be passed along as regular props:\r\n\r\n  ```js\r\n  View({cursor, outputChannel})\r\n  ```\r\n\r\n  In case we want to go that route we should probably change from just `.equals` to something less generic I'd say this:\r\n\r\n  ```js\r\n  var isEqual = require('lodash.isequal');\r\n\r\n  function isRebaseable(value) {\r\n    return value && value[isRebaseableOnto]\r\n  }\r\n\r\n  function isComparable(value) {\r\n    return value && typeof(value.equals) === 'function';\r\n  }\r\n\r\n  function isEqualComparable(expect, actual) {\r\n    return isComparable(expect) ? expect.equals(actual) :\r\n           isComparable(actual) ? false :\r\n           void 0;\r\n  }\r\n\r\n  function isRebaseableOntoOrEqual(expect, actual) {\r\n    return isRebaseable(expect) ? expect[isRebaseableOnto](actual) :\r\n              isEqualComparable(expect, actual);\r\n  }\r\n\r\n\r\n  function shouldComponentUpdate(nextProps, nextState) {\r\n    return !isEqual(nextProps, this.props, isRebaseableOntoOrEqual) ||\r\n           !isEqual(nextState, this.state, isEqualComparable);\r\n  }\r\n```\r\n3. As you could already have noticed in previous item this way we no longer need any special key in the `props` instead users can just wrap stuff they want to pass but do not affect `sholudComponentUpdate` and be done."},"comment":{"url":"https://api.github.com/repos/omniscientjs/omniscient/issues/comments/76520582","html_url":"https://github.com/omniscientjs/omniscient/issues/76#issuecomment-76520582","issue_url":"https://api.github.com/repos/omniscientjs/omniscient/issues/76","id":76520582,"user":{"login":"mikaelbr","id":606374,"avatar_url":"https://avatars.githubusercontent.com/u/606374?v=3","gravatar_id":"","url":"https://api.github.com/users/mikaelbr","html_url":"https://github.com/mikaelbr","followers_url":"https://api.github.com/users/mikaelbr/followers","following_url":"https://api.github.com/users/mikaelbr/following{/other_user}","gists_url":"https://api.github.com/users/mikaelbr/gists{/gist_id}","starred_url":"https://api.github.com/users/mikaelbr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mikaelbr/subscriptions","organizations_url":"https://api.github.com/users/mikaelbr/orgs","repos_url":"https://api.github.com/users/mikaelbr/repos","events_url":"https://api.github.com/users/mikaelbr/events{/privacy}","received_events_url":"https://api.github.com/users/mikaelbr/received_events","type":"User","site_admin":false},"created_at":"2015-02-28T10:27:34Z","updated_at":"2015-02-28T10:27:34Z","body":"@Gozala I've hadn't have the time to look at the details of this yet, but I just thought I'd say what the intention of `shouldComponentUpdate`, but I think we all pretty much agree that it should exit early and have some sort of precedence list for what is the cheapest and most likely checks first.\r\n\r\n> The fun fact is that _.isEqual on props actually needs to go and traverse complete structure unlike .equal on immutable data structures but that seems to be ok while later isn't :(\r\n\r\n`_.isEqual` would work as a generic solution (across cursors, immutable structures and object literals), and with `customizer` checking for immutable/cursors, we'd get the performance gains for them as well. So, the way I see it, `_.isEqual`, could probably use `.equal` internally in the `customizer`. \r\n\r\n>To cut to the point what I am saying is .equal on immutable data structures most certainly will be faster than triggering render unless you pass giant structures with no sharing while not using almost any of it. All this also can be also checked with a profiler to draw conclusions based on numbers.\r\n\r\nI haven't looked at the implementation of `.equal`, yet, but from what you're saying, it seems like it should be.\r\n\r\nI'll look more into this later."}}