Centralized handling shouldComponentUpdate into SchemaField #490
Conversation
src/components/fields/SchemaField.js
Outdated
| // idSchema and errorSchema will be equal | ||
| delete _props.idSchema; | ||
| delete _nextProps.idSchema; | ||
| return !deepEquals(_props, _nextProps); |
There was a problem hiding this comment.
I'm confused with this code:
- the implementation doesn't reflect what the comment says; we're not dropping
errorSchema; - the comment suggests we should only compare changes for
schemaandformData, which is not what we're doing; - I suspect we should take care of
uiSchemachanges as well.
I'm trying to summarize the points above in a sample implementation below:
shouldComponentUpdate(nextProps) {
return !deepEquals(this.props.formData, nextProps.formData) ||
!deepEquals(this.props.schema, nextProps.schema) ||
!deepEquals(this.props.uiSchema, nextProps.uiSchema);
}We may obviously want to ensure we're not missing other necessary checks, but I'm finding this way much more explicit.
There was a problem hiding this comment.
The comment suggests that "schema" and "formData" didn't change => "idSchema" and "errorSchema" didn't change. So, comparing "schema" and "formData" is enough to tell wheter "idSchema" and "errorSchema" have change or not.
But, not comparing errorSchema will fail some test. I haven't figure out the reason yet. And, the performance with comparing errorSchema looks acceptable.
I will update the comment.
src/components/fields/SchemaField.js
Outdated
| if (Object.keys(schema).length === 0) { | ||
| // See #312: Ensure compatibility with old versions of React. | ||
| return <div/>; | ||
| class SchemaField extends Component { |
There was a problem hiding this comment.
It's really sad there's no way to have the equivalent of shouldComponentUpdate for stateless components. I could totally see a decorator of some sort.
There was a problem hiding this comment.
I think React still need an instance to preserve props in order to have both "this.props" and "nextProps" to compare. recompose is one library I know to do such a thing. It will turn functions into classes.
test/performance_test.js
Outdated
|
|
||
| sinon.assert.notCalled(comp.render); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Why has this test been removed?
There was a problem hiding this comment.
The "shouldComponnetUpdate" of "ObjectField" and "ArrayField" were guaranteed by "SchemaField". So, I thought those two function were no longer necessary and the related tests were removed.
src/components/fields/SchemaField.js
Outdated
| ); | ||
| } | ||
|
|
||
| render(){ |
There was a problem hiding this comment.
nit: space before opening bracket
test/performance_test.js
Outdated
| sandbox.stub(fieldComp, "render").returns(<div/>); | ||
| fields[fieldComp.props.idSchema.$id] = fieldComp; | ||
| return fields; | ||
| }); |
There was a problem hiding this comment.
nit: let's apply a consistent coding style:
const fields = scryRenderedComponentsWithType(comp, SchemaField)
.reduce((fields, fieldComp) => {
sandbox.stub(fieldComp, "render").returns(<div/>);
fields[fieldComp.props.idSchema.$id] = fieldComp;
return fields;
});
|
Thank you 👍 |
New features * Add support for rows attribute of textarea widget. (#450) * #434 - Render empty array item fields when minItems is specified (#484) * Add a "has-danger" class to the form error list (#502) * Show description for boolean fields (#498) * Fix #488: Add a custom Form ErrorList prop. Bugfixes * Fix impossibility to use stateful ArrayFieldTeplate comp. (#519) * Centralized shouldComponentUpdate handling in SchemaField (#490)
|
Released in v0.44.0. |
Reasons for making this change
When one property is updated, the rest of the properties will be unnecessarily rendered.
This will cause a significant performance issue in a large object schema such as the demo of 'Large' in the playground.
Checklist