{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/255","repository_url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form","labels_url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/255/labels{/name}","comments_url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/255/comments","events_url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/255/events","html_url":"https://github.com/mozilla-services/react-jsonschema-form/issues/255","id":163422107,"number":255,"title":"Inefficiencies with shouldRender","user":{"login":"mplis-jetsetter","id":18580947,"avatar_url":"https://avatars.githubusercontent.com/u/18580947?v=3","gravatar_id":"","url":"https://api.github.com/users/mplis-jetsetter","html_url":"https://github.com/mplis-jetsetter","followers_url":"https://api.github.com/users/mplis-jetsetter/followers","following_url":"https://api.github.com/users/mplis-jetsetter/following{/other_user}","gists_url":"https://api.github.com/users/mplis-jetsetter/gists{/gist_id}","starred_url":"https://api.github.com/users/mplis-jetsetter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mplis-jetsetter/subscriptions","organizations_url":"https://api.github.com/users/mplis-jetsetter/orgs","repos_url":"https://api.github.com/users/mplis-jetsetter/repos","events_url":"https://api.github.com/users/mplis-jetsetter/events{/privacy}","received_events_url":"https://api.github.com/users/mplis-jetsetter/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-07-01T15:41:44Z","updated_at":"2016-07-02T10:06:42Z","closed_at":null,"body":"### Prerequisites\r\n\r\n- [x] I have read the [documentation](https://github.com/mozilla-services/react-jsonschema-form/blob/master/README.md#readme).\r\n\r\n### Description\r\n\r\nI believe the `shouldRender` function in `utils.js` is returning `true` in too many instances, particularly for nested `ArrayField`s or `ObjectField`s, because it's unnecessarily requiring function equality in props and state.\r\n\r\n### Steps to Reproduce\r\n\r\n1. In your clone of this repository, add this line to `shouldRender`: `console.log(comp.props, nextProps, !deeper(comp.props, nextProps) || !deeper(comp.state, nextState));`\r\n2. Start the development server with `npm start`\r\n3. Go to `localhost:8080` and click on the \"Nested\" tab\r\n4. Start typing in the \"Task list title\" field\r\n5. Look in the console logs for something like `Object {name: \"tasks\"...`. This is where `shouldRender` was called for the \"tasks\" `ArrayField`\r\n6. Observe that the objects logged on that line are structurally equivalent, but that `shouldRender` is still returning `true`. I believe that this is happening only because the `onChange` functions in those objects are referentially non-equal.\r\n\r\n#### Expected behavior\r\n\r\nI would expect that `shouldRender` for the \"tasks\" `ArrayField` would return `false` and that the component does not re-render, because none of the data that affects how the component renders has changed. Re-rendering all `ArrayField`s or `ObjectField`s when none of the data in that component has changed, particularly in large forms, is a performance drain.\r\n\r\n#### Actual behavior\r\n\r\n`ArrayField` re-renders when making a change to another part of the form.\r\n\r\n### Version\r\n\r\n0.34.1\r\n\r\n### Proposed Solution\r\n\r\nI think that a function like the one below should be added to `utils.js` and called on `comp.props` and `nextProps` before they're passed to `deeper`\r\n\r\n```\r\nfunction _removeFunctionsFromObj(obj) {\r\n  var newObj = {};\r\n  for (var key in obj) {\r\n    if (typeof obj[key] !== 'function') {\r\n      newObj[key] = obj[key];\r\n    }\r\n  }\r\n  return newObj;\r\n}\r\n```\r\n\r\nI'd be happy to submit a PR, but I thought it might be better to start a discussion first in case there's another way to solve this problem."},"comment":{"url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/comments/230094239","html_url":"https://github.com/mozilla-services/react-jsonschema-form/issues/255#issuecomment-230094239","issue_url":"https://api.github.com/repos/mozilla-services/react-jsonschema-form/issues/255","id":230094239,"user":{"login":"n1k0","id":41547,"avatar_url":"https://avatars.githubusercontent.com/u/41547?v=3","gravatar_id":"","url":"https://api.github.com/users/n1k0","html_url":"https://github.com/n1k0","followers_url":"https://api.github.com/users/n1k0/followers","following_url":"https://api.github.com/users/n1k0/following{/other_user}","gists_url":"https://api.github.com/users/n1k0/gists{/gist_id}","starred_url":"https://api.github.com/users/n1k0/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/n1k0/subscriptions","organizations_url":"https://api.github.com/users/n1k0/orgs","repos_url":"https://api.github.com/users/n1k0/repos","events_url":"https://api.github.com/users/n1k0/events{/privacy}","received_events_url":"https://api.github.com/users/n1k0/received_events","type":"User","site_admin":false},"created_at":"2016-07-02T10:06:42Z","updated_at":"2016-07-02T10:06:42Z","body":"> I'm not super familiar with ES6 classes yet, but is this related to why the Form's `onChange` method is defined like this\r\n\r\nYes. What you want to avoid is:\r\n\r\n```jsx\r\nrender() {\r\n  return <Child onChange={this.onChange.bind(this)} />;\r\n}\r\n```\r\n\r\nBecause that means mutating the reference to the function at each render, so the `onChange` would always be detected as different than previously:\r\n\r\n```js\r\n> foo.bind(null) === foo.bind(null)\r\nfalse\r\n> a = {}\r\nObject {}\r\n> foo.bind(a) === foo.bind(a)\r\nfalse\r\n```\r\n\r\nWith a class field, the method is already bound to the current instance so it's the very same function:\r\n\r\n```js\r\nclass Foo {\r\n  onChange = () => {}\r\n}\r\nconst foo = new Foo();\r\nfoo.onChange === foo.onChange // true\r\n```"}}
{"actor":{"display_login":"n1k0"}}