Skip to content

Remove setState in ObjectField and ArrayField #480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
n1k0 merged 4 commits into from
Feb 22, 2017
Merged

Remove setState in ObjectField and ArrayField #480

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8575a6f
Remove setState in ObjectField and ArrayField
knilink Feb 20, 2017
b8f07d5
Update performance test
knilink Feb 21, 2017
fb63caf
Fixed failing lint
knilink Feb 21, 2017
de766c2
Clean up.
knilink Feb 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 27 additions & 45 deletions src/components/fields/ArrayField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import {
retrieveSchema,
toIdSchema,
shouldRender,
getDefaultRegistry,
setState
getDefaultRegistry
} from "../../utils";

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that this is still needed elsewhere. If not, we should remove that helper function, which is now obsolete.


function ArrayFieldTitle({TitleField, idSchema, title, required}) {
Expand Down Expand Up @@ -151,6 +150,7 @@ function DefaultNormalArrayFieldTemplate(props) {
class ArrayField extends Component {
static defaultProps = {
uiSchema: {},
formData: [],
idSchema: {},
registry: getDefaultRegistry(),
required: false,
Expand All @@ -161,19 +161,6 @@ class ArrayField extends Component {

constructor(props) {
super(props);
this.state = this.getStateFromProps(props);
}

componentWillReceiveProps(nextProps) {
this.setState(this.getStateFromProps(nextProps));
}

getStateFromProps(props) {
const formData = Array.isArray(props.formData) ? props.formData : null;
const {definitions} = this.props.registry;
return {
items: getDefaultFormState(props.schema, formData, definitions) || []
};
}

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor is now unnecessary. Also that means we don't have local state anymore, which means we should use a functional stateless component instead of a component class. Which is so nice.

shouldComponentUpdate(nextProps, nextState) {
Expand All @@ -189,36 +176,29 @@ class ArrayField extends Component {
return itemsSchema.type === "string" && itemsSchema.minLength > 0;
}

asyncSetState(state, options={validate: false}) {
setState(this, state, () => {
this.props.onChange(this.state.items, options);
});
}

onAddClick = (event) => {
event.preventDefault();
const {items} = this.state;
const {schema, registry} = this.props;
const {schema, registry, formData} = this.props;
const {definitions} = registry;
let itemSchema = schema.items;
if (isFixedItems(schema) && allowAdditionalItems(schema)) {
itemSchema = schema.additionalItems;
}
this.asyncSetState({
items: items.concat([
getDefaultFormState(itemSchema, undefined, definitions)
])
});
this.props.onChange([
...formData,
getDefaultFormState(itemSchema, undefined, definitions)

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth renaming utils.getDefaultFormState which now sounds a little confusing. How about something like initDefaults?

@knilink knilink Feb 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether it is appropriate to do it at the moment because getDefaultFormState is used by the root Form component as well.

], {validate: false});
};

onDropIndexClick = (index) => {
return (event) => {
if (event) {
event.preventDefault();
}
this.asyncSetState({
items: this.state.items.filter((_, i) => i !== index)
}, {validate: true}); // refs #195
this.props.onChange(
this.props.formData.filter((_, i)=> i !== index),

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing space before =>.

{validate: true} // refs #195
);
};
};

Expand All @@ -228,33 +208,35 @@ class ArrayField extends Component {
event.preventDefault();
event.target.blur();
}
const {items} = this.state;
this.asyncSetState({
items: items.map((item, i) => {
const items = this.props.formData;

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can alternatively be written as const {formData: items} = this.props;, which I personally like a lot. But here we could probably just use formData as it makes sense that they're a list of items in an ArrayField.

this.props.onChange(
items.map((item, i) => {
if (i === newIndex) {
return items[index];
} else if (i === index) {
return items[newIndex];
} else {
return item;
}
})
}, {validate: true});
}),
{validate: true}
);
};
};

onChangeForIndex = (index) => {
return (value) => {
this.asyncSetState({
items: this.state.items.map((item, i) => {
this.props.onChange(
this.props.formData.map((item, i) => {
return index === i ? value : item;
})
});
}),
{validate: false}
);
};
};

onSelectChange = (value) => {
this.asyncSetState({items: value});
this.props.onChange(value, {validate: false});
};

render() {
Expand Down Expand Up @@ -287,7 +269,7 @@ class ArrayField extends Component {
onBlur
} = this.props;
const title = (schema.title === undefined) ? name : schema.title;
const {items = []} = this.state;
const items = this.props.formData;
const {ArrayFieldTemplate, definitions, fields} = registry;
const {TitleField, DescriptionField} = fields;
const itemsSchema = retrieveSchema(schema.items, definitions);
Expand Down Expand Up @@ -332,7 +314,7 @@ class ArrayField extends Component {

renderMultiSelect() {
const {schema, idSchema, uiSchema, disabled, readonly, autofocus, onBlur} = this.props;
const {items} = this.state;
const items = this.props.formData;
const {widgets, definitions, formContext} = this.props.registry;
const itemsSchema = retrieveSchema(schema.items, definitions);
const enumOptions = optionsList(itemsSchema);
Expand All @@ -357,7 +339,7 @@ class ArrayField extends Component {
renderFiles() {
const {schema, uiSchema, idSchema, name, disabled, readonly, autofocus, onBlur} = this.props;
const title = schema.title || name;
const {items} = this.state;
const items = this.props.formData;
const {widgets, formContext} = this.props.registry;
const {widget="files", ...options} = getUiOptions(uiSchema);
const Widget = getWidget(schema, widget, widgets);
Expand Down Expand Up @@ -393,7 +375,7 @@ class ArrayField extends Component {
onBlur
} = this.props;
const title = schema.title || name;
let {items} = this.state;
let items = this.props.formData;
const {ArrayFieldTemplate, definitions, fields} = registry;
const {TitleField} = fields;
const itemSchemas = schema.items.map(item =>
Expand Down
58 changes: 10 additions & 48 deletions src/components/fields/ObjectField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,16 @@
import React, {Component, PropTypes} from "react";

import {deepEquals} from "../../utils";


import {
getDefaultFormState,
orderProperties,
retrieveSchema,
shouldRender,
getDefaultRegistry,
setState
getDefaultRegistry
} from "../../utils";


function objectKeysHaveChanged(formData, state) {
// for performance, first check for lengths
const newKeys = Object.keys(formData);
const oldKeys = Object.keys(state);
if (newKeys.length < oldKeys.length) {
return true;
}
// deep check on sorted keys
if (!deepEquals(newKeys.sort(), oldKeys.sort())) {
return true;
}
return false;
}

class ObjectField extends Component {
static defaultProps = {
uiSchema: {},
formData: {},
errorSchema: {},
idSchema: {},
registry: getDefaultRegistry(),
Expand All @@ -40,25 +21,6 @@ class ObjectField extends Component {

constructor(props) {
super(props);
this.state = this.getStateFromProps(props);
}

componentWillReceiveProps(nextProps) {
const state = this.getStateFromProps(nextProps);
const {formData} = nextProps;
if (formData && objectKeysHaveChanged(formData, this.state)) {
// We *need* to replace state entirely here has we have received formData
// holding different keys (so with some removed).
this.state = state;
this.forceUpdate();
} else {
this.setState(state);
}
}

getStateFromProps(props) {
const {schema, formData, registry} = props;
return getDefaultFormState(schema, formData, registry.definitions) || {};
}

shouldComponentUpdate(nextProps, nextState) {
Expand All @@ -71,21 +33,21 @@ class ObjectField extends Component {
schema.required.indexOf(name) !== -1;
}

asyncSetState(state, options={validate: false}) {
setState(this, state, () => {
this.props.onChange(this.state, options);
});
}

onPropertyChange = (name) => {
return (value, options) => {
this.asyncSetState({[name]: value}, options);
const newFormData = Object.assign(
{},
this.props.formData,
{[name]: value}
);

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do:

const newFormData = {...this.props.formData, [name]: value};

this.props.onChange(newFormData, options);
};

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as with ArrayField, this should become a stateless component.

};

render() {
const {
uiSchema,
formData,
errorSchema,
idSchema,
name,

@n1k0 n1k0 Feb 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so happy seeing this going away 👍

@MoOx MoOx Feb 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too :)

Expand Down Expand Up @@ -135,7 +97,7 @@ class ObjectField extends Component {
uiSchema={uiSchema[name]}
errorSchema={errorSchema[name]}
idSchema={idSchema[name]}
formData={this.state[name]}
formData={formData[name]}
onChange={this.onPropertyChange(name)}
onBlur={onBlur}
registry={this.props.registry}
Expand Down
43 changes: 24 additions & 19 deletions test/performance_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import ObjectField from "../src/components/fields/ObjectField";
import {
createComponent,
createFormComponent,
createSandbox
createSandbox,
setProps
} from "./test_utils";


Expand Down Expand Up @@ -56,33 +57,35 @@ describe("Rendering performance optimizations", () => {
const registry = getDefaultRegistry();

it("should not render if next props are equivalent", () => {
const {comp} = createComponent(ArrayField, {
const props = {
registry,
schema,
uiSchema,
onChange,
onBlur
});
};

const {comp} = createComponent(ArrayField, props);
sandbox.stub(comp, "render").returns(<div/>);

comp.componentWillReceiveProps({schema});
setProps(comp, props);

sinon.assert.notCalled(comp.render);
});

it("should not render if next formData are equivalent", () => {
const formData = ["a", "b"];

const {comp} = createComponent(ArrayField, {
const props = {
registry,
schema,
formData,
formData: ["a", "b"],
onChange,
onBlur
});
};

const {comp} = createComponent(ArrayField, props);
sandbox.stub(comp, "render").returns(<div/>);

comp.componentWillReceiveProps({schema, formData});
setProps(comp, {...props, formData: ["a", "b"]});

sinon.assert.notCalled(comp.render);
});
Expand All @@ -102,35 +105,37 @@ describe("Rendering performance optimizations", () => {
const idSchema = {$id: "root", foo: {$id: "root_plop"}};

it("should not render if next props are equivalent", () => {
const {comp} = createComponent(ObjectField, {
const props = {
registry,
schema,
uiSchema,
onChange,
idSchema,
onBlur
});
};

const {comp} = createComponent(ObjectField, props);
sandbox.stub(comp, "render").returns(<div/>);

comp.componentWillReceiveProps({schema, registry});
setProps(comp, props);

sinon.assert.notCalled(comp.render);
});

it("should not render if next formData are equivalent", () => {
const formData = {foo: "blah"};

const {comp} = createComponent(ObjectField, {
const props = {
registry,
schema,
formData,
formData: {foo: "blah"},
onChange,
idSchema,
onBlur
});
};

const {comp} = createComponent(ObjectField, props);
sandbox.stub(comp, "render").returns(<div/>);

comp.componentWillReceiveProps({schema, formData, registry});
setProps(comp, {...props, formData: {foo: "blah"}});

sinon.assert.notCalled(comp.render);
});
Expand Down
12 changes: 11 additions & 1 deletion test/test_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import React from "react";
import sinon from "sinon";
import {renderIntoDocument} from "react-addons-test-utils";
import {findDOMNode} from "react-dom";
import {findDOMNode, render} from "react-dom";

import Form from "../src";

Expand All @@ -26,3 +26,13 @@ export function createSandbox() {
});
return sandbox;
}

export function setProps(comp, newProps) {
const node = findDOMNode(comp);
render(
React.createElement(
comp.constructor,
newProps
),
node.parentNode);
}