{"action":"created","issue":{"url":"https://api.github.com/repos/kadirahq/react-komposer/issues/99","repository_url":"https://api.github.com/repos/kadirahq/react-komposer","labels_url":"https://api.github.com/repos/kadirahq/react-komposer/issues/99/labels{/name}","comments_url":"https://api.github.com/repos/kadirahq/react-komposer/issues/99/comments","events_url":"https://api.github.com/repos/kadirahq/react-komposer/issues/99/events","html_url":"https://github.com/kadirahq/react-komposer/pull/99","id":163745448,"number":99,"title":"shouldResubscribe option","user":{"login":"damonmaria","id":1654191,"avatar_url":"https://avatars.githubusercontent.com/u/1654191?v=3","gravatar_id":"","url":"https://api.github.com/users/damonmaria","html_url":"https://github.com/damonmaria","followers_url":"https://api.github.com/users/damonmaria/followers","following_url":"https://api.github.com/users/damonmaria/following{/other_user}","gists_url":"https://api.github.com/users/damonmaria/gists{/gist_id}","starred_url":"https://api.github.com/users/damonmaria/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/damonmaria/subscriptions","organizations_url":"https://api.github.com/users/damonmaria/orgs","repos_url":"https://api.github.com/users/damonmaria/repos","events_url":"https://api.github.com/users/damonmaria/events{/privacy}","received_events_url":"https://api.github.com/users/damonmaria/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":13,"created_at":"2016-07-04T21:43:48Z","updated_at":"2016-07-27T18:23:46Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/kadirahq/react-komposer/pulls/99","html_url":"https://github.com/kadirahq/react-komposer/pull/99","diff_url":"https://github.com/kadirahq/react-komposer/pull/99.diff","patch_url":"https://github.com/kadirahq/react-komposer/pull/99.patch"},"body":"## The problem\r\nThe main reason for this PR is I have a couple of cases where compose ends up in an infinite loop. It goes something like this:\r\n\r\n1. On first call to my `onPropsChange` I start an async data fetch and fire `onData` which shows a contained component in one style\r\n2. Later ,once data arrives, `onData` is fired again and the component changes look drastically\r\n3. This seems to cause React to fire `componentWillReceiveProps` with the same prop values as before\r\n4. react-komposer then calls my `onPropsChange`, and so we go back to step one\r\n5. Repeat forever grinding that browser tab to a halt\r\n\r\nSee the [note in the React docs](https://facebook.github.io/react/docs/component-specs.html#updating-componentwillreceiveprops) of `componentWillRecieveProps` for an explanation that this being fired with the same props is expected.\r\n\r\nAt a higher level, sometimes (often) the lifecycle of the component has to be taken into account in `onPropsChange`. Many users will expect it to only fire once per component. \r\n\r\nPreviously there was no way to detect that `onPropsChange` is being called with the same props and the expensive / async operation does not need to be repeated. \r\n\r\n## What this PR does\r\n1. The current `!pure` and `!shallowEqual` check for the props is done in `componentWillRecieveProps` _before_ `onPropsChange` is fired, not just in `shouldComponentUpdate`. This will stop a lot of existing redundent calls to `onPropsChange` but might somehow cause a backwards incompatible change if current react-komposer users have impure `onProposChange` functions but are not setting `pure: false` (not very likely IMHO). \r\n2. The above check can be overriden with a custom function passed in the options as `shouldResubscribe`. This allows the user to check for a change in only the props that affect the `onPropsChange` function. This is useful, for example, when there are multiple layers of `compose` components and props coming down from outer components then don't need to fire `onProposChange` on unrelated inner components.\r\n\r\n## Other implementation paths not taken\r\nI thought about binding `this` to the component so cache check info could be kept on the component and checked by `onPropsChange` but that breaks the pure function principle that this package aims for. Same with having the same cache info in a variable outside the `onPropsChange` function (that also has bugs in certain scenarios if mutliple components are used). \r\n\r\nI also thought about passing the old props into `onPropsChange` but that function already has enough parameters and it didn't seem good separation of concerns. \r\n\r\n## Tests / docs\r\nPlenty of tests and API doc added. \r\n\r\nAlso added `env: \"mocha\"` to `.eslintrc` otherwise ESlint bawked on `beforeEach()`."},"comment":{"url":"https://api.github.com/repos/kadirahq/react-komposer/issues/comments/235674902","html_url":"https://github.com/kadirahq/react-komposer/pull/99#issuecomment-235674902","issue_url":"https://api.github.com/repos/kadirahq/react-komposer/issues/99","id":235674902,"user":{"login":"markoshust","id":437029,"avatar_url":"https://avatars.githubusercontent.com/u/437029?v=3","gravatar_id":"","url":"https://api.github.com/users/markoshust","html_url":"https://github.com/markoshust","followers_url":"https://api.github.com/users/markoshust/followers","following_url":"https://api.github.com/users/markoshust/following{/other_user}","gists_url":"https://api.github.com/users/markoshust/gists{/gist_id}","starred_url":"https://api.github.com/users/markoshust/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/markoshust/subscriptions","organizations_url":"https://api.github.com/users/markoshust/orgs","repos_url":"https://api.github.com/users/markoshust/repos","events_url":"https://api.github.com/users/markoshust/events{/privacy}","received_events_url":"https://api.github.com/users/markoshust/received_events","type":"User","site_admin":false},"created_at":"2016-07-27T18:23:46Z","updated_at":"2016-07-27T18:23:46Z","body":"The ticket at https://github.com/mobxjs/mobx/issues/84#issuecomment-235652897 explains a good use-case for merging this in..."}}
{"actor":{"display_login":"markoshust"}}