Remove setState in ObjectField and ArrayField #480

knilink · 2017-02-21T01:03:40Z

Reasons for making this change

The preserving state in "ObjectField" and "ArrayField" was unnecessary because the root Form component had done the job(Form.js line 152). It may be the root cause of #339. (Haven't test it in IE but it did fix my losing characters issue).

Performance test has been updated due to componentWillReceiveProps in "ObjectField" and "ArrayField" are no longer necessary and have been removed.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

n1k0

Okay, I'm sold. I'm pretty much amazed by this patch as it highlights how much unnecessary bloat was involved previously. I've tested the playground with your changes and couldn't spot any noticeable major behavioral change apart from a slight increase in performance and reactivity, which is awesome.

A few nits here and there and we're probably good to go.

Thanks a lot for your work 👍

n1k0 · 2017-02-21T07:40:01Z

src/components/fields/ObjectField.js

+        {},
+        this.props.formData,
+        {[name]: value}
+      );


You can do:

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

n1k0 · 2017-02-21T07:44:07Z

src/components/fields/ArrayField.js

@@ -161,19 +161,6 @@ class ArrayField extends Component {

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


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.

n1k0 · 2017-02-21T07:45:08Z

src/components/fields/ObjectField.js

@@ -40,25 +21,6 @@ class ObjectField extends Component {

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


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

n1k0 · 2017-02-21T07:49:35Z

src/components/fields/ArrayField.js

-    });
+    this.props.onChange([
+      ...formData,
+      getDefaultFormState(itemSchema, undefined, definitions)


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

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.

n1k0 · 2017-02-21T07:50:23Z

src/components/fields/ArrayField.js

-        items: this.state.items.filter((_, i) => i !== index)
-      }, {validate: true}); // refs #195
+      this.props.onChange(
+        this.props.formData.filter((_, i)=> i !== index),


Nit: missing space before =>.

n1k0 · 2017-02-21T07:51:29Z

src/components/fields/ArrayField.js

-      const {items} = this.state;
-      this.asyncSetState({
-        items: items.map((item, i) => {
+      const items = this.props.formData;


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.

n1k0 · 2017-02-21T07:53:57Z

src/components/fields/ArrayField.js

@@ -12,8 +12,7 @@ import {
  retrieveSchema,
  toIdSchema,
  shouldRender,
-  getDefaultRegistry,
-  setState


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

n1k0 · 2017-02-21T07:55:01Z

src/components/fields/ObjectField.js

-      // We *need* to replace state entirely here has we have received formData
-      // holding different keys (so with some removed).
-      this.state = state;
-      this.forceUpdate();


I'm so happy seeing this going away 👍

Mitgenosse · 2017-02-21T16:15:55Z

Nice work, I hope this fixes #399 .
Can you tell me when this will be merged and published?

n1k0 · 2017-02-21T16:32:56Z

@Gurkenschreck Once review comments are addressed.

Nice work, I hope this fixes #399 .

Maybe you could clone this branch and test it locally to see if it fixes/improves #339 (which is the issue you actually want to mention here I think)? That would give us useful feedback. Thanks!

n1k0 · 2017-02-22T07:53:01Z

👍

Mitgenosse · 2017-02-22T07:58:50Z

Yes I can do that.
EDIT: It actually works locally on my IE11 Virtual Machine. Gonna try out IE9

MoOx · 2017-02-22T11:10:46Z

Looks fantastic according to my test!

stathismor · 2017-02-22T11:21:38Z

If/when I get around adding support for anyOf as part of #417, I think I will need to bring the state back, but probably only using it for ArrayField's internal own stuff, and not for getting the formData as the code before this PR does.

Mitgenosse · 2017-02-22T14:14:37Z

Is anywhere stated what browsers are supported? The branch gh-pages cannot be seen in IE9.

Console output of https://mozilla-services.github.io/react-jsonschema-form/ in IE9

SCRIPT438: Object doesn't support property or method 'bind' 
bundle.js, line 1 character 226

I would raise an issue if IE9 should be supported.

n1k0 · 2017-02-22T14:28:06Z

We're not planning on supporting IE9. It looks like it could be achieved by including the right polyfills though, but I'd like to let developers (lib users) taking care of this.

But you're right we should document what our current target browser versions are.

@crumblix

Highlights --- - Improved performance and reactivity. - More consistent validation behavior and UX for array field items. Backward incompatible changes --- - `ObjectField` and `ArrayField` are now stateless components, their local state handling has been dropped entirely, resulting in large performance improvements. - The `defaultFieldValue` helper from the `utils` module has been removed, as it wasn't used anymore. New features --- * Fix #411: Enable required field on radio options. (#469) * Spread `formContext` to `ArrayTemplateField`, `files` and `multiselect` arrays (#456) * From #471: Non-nullable array item fields are now marked as required in the UI. Bugfixes --- * Don't pass consumed `SchemaField` class names to child component (#439) * Turn `ObjectField` and `ArrayField` into stateless components (#480) * Fix #471: Drop default value initialization at the widget level. (#476) Kudos --- Special thanks to @crumblix and @knilink for their help on this release. You guys rock!

n1k0 · 2017-02-22T16:41:02Z

Released in v0.43.0.

combatpoodle · 2017-03-17T18:07:31Z

<3

knilink added 3 commits February 20, 2017 17:09

Remove setState in ObjectField and ArrayField

8575a6f

Update performance test

b8f07d5

Fixed failing lint

fb63caf

n1k0 reviewed Feb 21, 2017

View reviewed changes

Clean up.

de766c2

n1k0 merged commit 4b6fe72 into rjsf-team:master Feb 22, 2017

n1k0 mentioned this pull request Feb 22, 2017

Loses characters on Internet Explorer 11 #339

Closed

This was referenced Feb 22, 2017

On slow CPUs safeRenderCompletion needs to be set to make sure RJSF waits for setState before calling onChange #446

Closed

Perf waste #424

Closed

jlame mentioned this pull request Jun 12, 2017

fix(forms): change perf issue (skipped char on IE) Talend/ui#524

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove setState in ObjectField and ArrayField #480

Remove setState in ObjectField and ArrayField #480

knilink commented Feb 21, 2017 •

edited

Loading

n1k0 left a comment

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

knilink Feb 22, 2017

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

n1k0 Feb 21, 2017

MoOx Feb 22, 2017

Mitgenosse commented Feb 21, 2017

n1k0 commented Feb 21, 2017 •

edited

Loading

n1k0 commented Feb 22, 2017

Mitgenosse commented Feb 22, 2017 •

edited

Loading

MoOx commented Feb 22, 2017

stathismor commented Feb 22, 2017

Mitgenosse commented Feb 22, 2017

n1k0 commented Feb 22, 2017 •

edited

Loading

n1k0 commented Feb 22, 2017

combatpoodle commented Mar 17, 2017

Remove setState in ObjectField and ArrayField #480

Remove setState in ObjectField and ArrayField #480

Conversation

knilink commented Feb 21, 2017 • edited Loading

Reasons for making this change

Checklist

n1k0 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mitgenosse commented Feb 21, 2017

n1k0 commented Feb 21, 2017 • edited Loading

n1k0 commented Feb 22, 2017

Mitgenosse commented Feb 22, 2017 • edited Loading

MoOx commented Feb 22, 2017

stathismor commented Feb 22, 2017

Mitgenosse commented Feb 22, 2017

n1k0 commented Feb 22, 2017 • edited Loading

n1k0 commented Feb 22, 2017

combatpoodle commented Mar 17, 2017

knilink commented Feb 21, 2017 •

edited

Loading

n1k0 commented Feb 21, 2017 •

edited

Loading

Mitgenosse commented Feb 22, 2017 •

edited

Loading

n1k0 commented Feb 22, 2017 •

edited

Loading