Use shallowEqual in shouldComponentUpdate #940

leesiongchan · 2016-05-11T06:17:44Z

Do you think it makes sense to use shallowEqual or even remove the entire shouldComponentUpdate as Field should be treated as a simple component to connects form's state and so shouldComponentUpdate shouldn't be part of the job.

Fixes #936

ooflorent · 2016-05-11T07:10:25Z

src/Field.js

@@ -1,6 +1,7 @@
 import React, { Component, PropTypes } from 'react'
 import invariant from 'invariant'
 import createConnectedField from './ConnectedField'
+import shallowEqual from 'fbjs/lib/shallowEqual'


fbjs isn't a redux-form dependency. You can access it because you are using npm@3.

ooflorent · 2016-05-11T07:16:41Z

One drawback of this change is that it will rerender a lot more!
Let's consider the following Field:

render() {
  return (
    <Field name="color" component="select">
      <option value="r">Red</option>
      <option value="g">Green</option>
      <option value="b">Blue</option>
    </Field>
  )
}

When render will be called, the Field's children will change but still represent the same VDOM and a rerender will occur.

erikras · 2016-05-11T07:19:58Z

I'm fine with a dep on shallowequal.

Please do FieldArray too.

erikras · 2016-05-11T07:22:21Z

When render will be called, the Field's children will change but still represent the same VDOM and a rerender will occur.

Perhaps children could be excluded from the shallow equal check?

The tests should be sure that it only rerenders once when the otherProp changes.

ooflorent · 2016-05-11T07:23:48Z

@erikras I think react-addons-shallow-compare would be better. It is widely used and so would virtually not enlarge the lib size.

ooflorent · 2016-05-11T07:26:11Z

Perhaps children could be excluded from the shallow equal check?

Same behavior when using a render-time created closure for component. If this change is shipped, it would be a good idea to add a note to the documentation.

leesiongchan · 2016-05-11T08:22:21Z

@ooflorent react-addons-shallow-compare takes 3 parameters which include nextState as well, does it really fit into our need?

ooflorent · 2016-05-11T08:28:29Z

@leesiongchan Since Field and FieldArray don't have state, it doesn't matter. Shallow equal on undefined should be fast enough.

What to you think @erikras about the addon?

erikras · 2016-05-11T08:36:10Z

What to you think @erikras about the add-on?

👍 Sounds good.

leesiongchan · 2016-05-11T08:57:49Z

But this change to FieldArray will fail the test as ConnectedFieldArray is comparing using the size of its value. Are we going to use deepEqual like ConnectField does?

ooflorent · 2016-05-11T09:10:00Z

package.json

@@ -70,6 +70,7 @@
    "lodash-webpack-plugin": "^0.3.0",
    "mocha": "^2.4.5",
    "react": "^15.0.0",
+    "react-addons-shallow-compare": "^15.0.0",


Should be added to peerDependencies as well, or dependencies. I don't know what's the best practice on this...

ping @erikras

This is critical. If you don't add it to dependencies or peerDependencies, the package will be broken.

erikras · 2016-05-11T09:15:09Z

But this change to FieldArray will fail the test as ConnectedFieldArray is comparing using the size of its value. Are we going to use deepEqual like ConnectedField does?

No, ConnectedFieldArray should just use shallow equals. We don't want it rerendering the whole array as you type into one of the inputs.

leesiongchan · 2016-05-11T09:17:42Z

Updated.

leesiongchan · 2016-05-11T09:26:49Z

package.json

@@ -43,7 +43,8 @@
    "hoist-non-react-statics": "^1.0.5",
    "invariant": "^2.2.1",
    "is-promise": "^2.1.0",
-    "lodash": "^4.12.0"
+    "lodash": "^4.12.0",
+    "react-addons-shallow-compare": "^15.0.0"


@ooflorent I think is more appropriate to add to dependency compare to peer. @erikras Let me know if I am wrong.

I'm no npm expert, but I would think it belongs in dependencies.

leesiongchan · 2016-05-11T14:23:20Z

Updated.

ooflorent · 2016-05-11T14:28:36Z

src/ConnectedFieldArray.js

@@ -23,7 +24,7 @@ const createConnectedFieldArray = ({

  class ConnectedFieldArray extends Component {
    shouldComponentUpdate(nextProps) {
-      return size(this.props.value) !== size(nextProps.value)
+      return shallowCompare(this, nextProps)


Are you sure for this one? cc @erikras

Discussed on above. Check here

I'm pretty sure. I think this will fix #931.

@leesiongchan, can you write a test where there is a _error validation error for the array based on some quality of the values inside the array (e.g. if none of them have values), and verify that the array component rerenders when the error appears or disappears?

@erikras Are you saying something like this? Link

erikras · 2016-05-12T20:00:11Z

Published as v6.0.0-alpha.9. Thanks, @leesiongchan!

erikras · 2016-05-12T22:27:37Z

@leesiongchan and @ooflorent, as you may get an earlier start on your Friday than I...

The examples have been giving me Uncaught Error: Cannot find module "react-addons-shallow-compare". I looked up the code in node_modules/react-addons-shallow-compare, and it was literally just require()ing react/lib/shallowCompare, so, out of desperation, I tried just importing react/lib/shallowCompare instead of react-addons-shallow-compare, but I am still getting the same error message. e.g. 6.0.0-alpha.10 Simple Example. Do you have any insights into what the problem might be? I cannot find any references to react-addons-shallow-compare anywhere in the project.

Thanks!

leesiongchan · 2016-05-13T04:48:21Z

Can't see the problem too :'(

ooflorent · 2016-05-13T06:23:54Z

@erikras I can't reproduce the issue. I did a fresh install and ran the builds and test suites and everything worked fine.

erikras · 2016-05-13T06:51:36Z

Okay. I swear I was rm -rfing all of the node_modules directories last night, but I'll try with a fresh git clone this morning to be absolutely sure.

Thanks for confirming my insanity. 😄 👍

ooflorent · 2016-05-13T07:01:22Z

Maybe you were hitting Babel's cache of webpack's one

lock · 2018-06-02T05:55:25Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ooflorent reviewed May 11, 2016
View reviewed changes

leesiongchan force-pushed the v6-shallowEqual branch from 61ade5c to edf1fe2 Compare May 11, 2016 09:06

ooflorent reviewed May 11, 2016
View reviewed changes

leesiongchan force-pushed the v6-shallowEqual branch from edf1fe2 to bc5e87c Compare May 11, 2016 09:17

leesiongchan changed the title ~~Use shallowEqual in Field's shouldComponentUpdate~~ Use shallowEqual in shouldComponentUpdate May 11, 2016

leesiongchan force-pushed the v6-shallowEqual branch from bc5e87c to e688126 Compare May 11, 2016 09:25

leesiongchan reviewed May 11, 2016
View reviewed changes

leesiongchan force-pushed the v6-shallowEqual branch from e688126 to f3cd1ea Compare May 11, 2016 14:23

ooflorent reviewed May 11, 2016
View reviewed changes

leesiongchan force-pushed the v6-shallowEqual branch from f3cd1ea to 8d6090a Compare May 12, 2016 02:44

Use shallowEqual in shouldComponentUpdate

da6720c

leesiongchan force-pushed the v6-shallowEqual branch from 8d6090a to da6720c Compare May 12, 2016 09:22

ooflorent mentioned this pull request May 12, 2016

v6: Field does not rerender on prop updates #948

Closed

erikras merged commit e605d94 into redux-form:v6 May 12, 2016

leesiongchan deleted the v6-shallowEqual branch May 13, 2016 02:28

This was referenced May 13, 2016

v6 (alpha 9): Some input fields lose focus after typing #961

Closed

v6 Fields getting remounted in FieldArray #969

Closed

How can I switch the disabling of the form fields using v 6 Field or FieldArray? #976

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use shallowEqual in shouldComponentUpdate #940

Use shallowEqual in shouldComponentUpdate #940

leesiongchan commented May 11, 2016

ooflorent May 11, 2016 •

edited

ooflorent commented May 11, 2016

erikras commented May 11, 2016

erikras commented May 11, 2016

ooflorent commented May 11, 2016

ooflorent commented May 11, 2016 •

edited

leesiongchan commented May 11, 2016 •

edited

ooflorent commented May 11, 2016 •

edited

erikras commented May 11, 2016

leesiongchan commented May 11, 2016

ooflorent May 11, 2016 •

edited

leesiongchan May 11, 2016

ooflorent May 11, 2016

erikras commented May 11, 2016

leesiongchan commented May 11, 2016

leesiongchan May 11, 2016

erikras May 11, 2016

leesiongchan commented May 11, 2016

ooflorent May 11, 2016

leesiongchan May 11, 2016

erikras May 11, 2016

leesiongchan May 12, 2016

erikras May 12, 2016

erikras commented May 12, 2016

erikras commented May 12, 2016

leesiongchan commented May 13, 2016

ooflorent commented May 13, 2016

erikras commented May 13, 2016

ooflorent commented May 13, 2016

lock bot commented Jun 2, 2018

Use shallowEqual in shouldComponentUpdate #940

Use shallowEqual in shouldComponentUpdate #940

Conversation

leesiongchan commented May 11, 2016

ooflorent May 11, 2016 • edited

Choose a reason for hiding this comment

ooflorent commented May 11, 2016

erikras commented May 11, 2016

erikras commented May 11, 2016

ooflorent commented May 11, 2016

ooflorent commented May 11, 2016 • edited

leesiongchan commented May 11, 2016 • edited

ooflorent commented May 11, 2016 • edited

erikras commented May 11, 2016

leesiongchan commented May 11, 2016

ooflorent May 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikras commented May 11, 2016

leesiongchan commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesiongchan commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikras commented May 12, 2016

erikras commented May 12, 2016

leesiongchan commented May 13, 2016

ooflorent commented May 13, 2016

erikras commented May 13, 2016

ooflorent commented May 13, 2016

lock bot commented Jun 2, 2018

ooflorent May 11, 2016 •

edited

ooflorent commented May 11, 2016 •

edited

leesiongchan commented May 11, 2016 •

edited

ooflorent commented May 11, 2016 •

edited

ooflorent May 11, 2016 •

edited