Removed setState from util #1454

mirajp · 2019-09-08T18:51:19Z

Removed setSstate from util

Reasons for making this change

Remove the util setState call from Form.onChange because it sets the state too early or something? Hard to describe, I came upon this fix (for #1281) after trying a bunch of things like using a leaner componentDidUpdate and removing shouldComponentUpdate (so it uses the default React Component function which always returns true) instead of the existing componentWillReceiveProps and shouldComponentUpdate. Note that removing componentWillReceiveProps breaks a lot of existing tests because for some reason they rely on componentWillReceiveProps (not sure why tests call that function directly, but I'm just a little more used to Jest than the raw 'react-addons-test-utils' library), so that's not an option anyway.

I'm not sure how to add tests for the bug I'm trying to fix, so feel free to edit this PR.

If this is related to existing tickets, include links to them as well.
#1281

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
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

Fixed typo

epicfaace

Thanks! Can you also remove all instances of safeRenderCompletion since it no longer has an effect?

epicfaace · 2019-09-09T20:25:56Z

Actually, master now has the v2 code, so I've changed the base.

mirajp · 2019-09-10T03:27:16Z

@epicfaace good catch. I removed safeRenderCompletion usages. Made mistake with commit message because I was rereading #1197 -- I believe setImmediate is still necessary to wait for DOM changes, though perhaps setTimeout could work in its place, e.g.:

/* ArrayField_test.js */

// existing setImmediate usage
new Promise(setImmediate).then(() =>
    expect(comp.state.formData).eql([
        'data:text/plain;name=file1.txt;base64,x=',
        'data:text/plain;name=file2.txt;base64,x=',
    ])
);

// can be replaced with:
setTimeout(() => {
    expect(comp.state.formData).eql([
        'data:text/plain;name=file1.txt;base64,x=',
        'data:text/plain;name=file2.txt;base64,x=',
    ]);
}, 100);

mirajp · 2019-09-11T02:57:19Z

@epicfaace not sure why Travis build agents are failing -- all tests pass locally for me (Windows + WSL).

epicfaace · 2019-09-11T04:02:18Z

Can you merge the latest master into this branch? That will take care of most of the failures. There's still a single build failure, but that's on node 6 and not sure why it's happening (https://github.com/rjsf-team/react-jsonschema-form/runs/217099766)

Merge in upstream master

mirajp · 2019-09-11T04:16:44Z

@epicfaace I actually have quite a few test failures (locally) once I've merged in master (node v10.16.0)... If only node v6 is failing, I don't think it's too much of a big deal as it's at EOL (https://raw.githubusercontent.com/nodejs/Release/master/schedule.svg?sanitize=true)? I recall getting random issues with node v6 at work and I pretty much just gave up trying to support it and changed all pipelines to only use v10 ;)

Also, shouldn't the pipeline be building/testing the attempted merge (i.e. the projected state with master + feature)? I don't really get how the PR build produced a failure when neither master nor the feature branch without master failed...

Anyway, here are some of the failures I'm seeing locally now (in master and now my branch which has master):

1183 passing (5s)

  14 failing

  1) anyOf

       should select the correct field when the formData property is updated:

      AssertionError: expected '0' to deeply equal '1'

      + expected - actual

      -0

      +1

      at Context.eql (test/anyOf_test.js:391:48)

  2) ArrayField

       Fixed items lists

         operations for additional items

           should remove array items when clicking remove buttons:

      AssertionError: expected [ 1, 2, undefined ] to deeply equal [ 1, 2, 'baz' ]

      + expected - actual

       [

         1

         2

      -  [undefined]

      +  "baz"

       ]

      at Context.eql (test/ArrayField_test.js:1576:37)

...

  9) NumberField

       Number and text widget

         should update input values correctly when formData prop changes:

      AssertionError: expected '2.03' to deeply equal '203'

      + expected - actual

      -2.03

      +203

      at Context.eql (test/NumberField_test.js:307:30)

  10) NumberField

       Number and text widget

         should update input values correctly when formData prop changes:

      AssertionError: expected '2.03' to deeply equal '203'

      + expected - actual

      -2.03

      +203

      at Context.eql (test/NumberField_test.js:307:30)

  11) oneOf

       should select the correct field when the formData property is updated:

      AssertionError: expected '0' to deeply equal '1'

      + expected - actual

      -0

      +1

      at Context.eql (test/oneOf_test.js:391:48)

...

  13) Rendering performance optimizations

       Form

         should only render changed array items:

     AssertError: expected render to be called once but was called 0 times

      at Object.fail (node_modules\sinon\lib\sinon\assert.js:110:29)

      at failAssertion (node_modules\sinon\lib\sinon\assert.js:69:24)

      at Object.assert.(anonymous function) [as calledOnce] (node_modules\sinon\lib\sinon\assert.js:94:21)

      at Context.calledOnce (test/performance_test.js:99:20)

  14) utils

       getWidget()

         should not fail on forwarded ref component:

     TypeError: _react.default.forwardRef is not a function

      at Context.forwardRef (test/utils_test.js:2878:28)

chanceaclark · 2019-09-11T15:58:34Z

If Node 6 is out of support, rjsf should at least support Node 8+. Is there any benefit to supporting 6?

With @epicfaace most recent commit onto master the only issue I can see is on Node 6 with an AssertionError

Anyway, here are some of the failures I'm seeing locally now (in master and now my branch which has master):

For clarification, are you using node 6 with these errors?

epicfaace · 2019-09-11T17:11:45Z

Also, shouldn't the pipeline be building/testing the attempted merge (i.e. the projected state with master + feature)? I don't really get how the PR build produced a failure when neither master nor the feature branch without master failed...

Master is failing, although not with the errors you mentioned. Master failed in the first place because I cherry-picked #1408 from v2 to master -- I probably should have made a PR to make sure the tests passed first.

epicfaace · 2019-09-11T17:13:30Z

If Node 6 is out of support, rjsf should at least support Node 8+. Is there any benefit to supporting 6?

Yeah, I think you're right. It was preferable to keep support for Node 6 in 1.x to keep backwards compatibility, but at this point we can change it to Node 8+.

chanceaclark · 2019-09-11T18:30:59Z

I'll go ahead and submit a PR then.

epicfaace · 2019-11-01T03:54:26Z

test/test_utils.js

@@ -14,7 +14,7 @@ export function createComponent(Component, props) {
 }

 export function createFormComponent(props) {
-  return createComponent(Form, { ...props, safeRenderCompletion: true });
+  return createComponent(Form, { ...props });


Suggested change

return createComponent(Form, { ...props });

return createComponent(Form, props);

epicfaace · 2019-11-01T03:57:01Z

Thanks @mirajp !

Removed setState from util

0f2dead

Fixed typo

mirajp mentioned this pull request Sep 8, 2019

Remove setState util call in Form.onChange #1297

Closed

8 tasks

epicfaace reviewed Sep 9, 2019

View reviewed changes

epicfaace changed the base branch from v2 to master September 9, 2019 20:25

Removed 'setImmediate' prop usages

55fd823

mirajp and others added 2 commits September 10, 2019 21:06

Merge pull request #1 from rjsf-team/master

bb92a05

Merge in upstream master

Merge branch 'master' into remove_setState_util

321866f

chanceaclark mentioned this pull request Sep 11, 2019

chore: remove node version < 8 from ci #1462

Merged

epicfaace reviewed Nov 1, 2019

View reviewed changes

Merge branch 'master' into remove_setState_util

11c52a3

epicfaace merged commit 635df88 into rjsf-team:master Nov 1, 2019

epicfaace mentioned this pull request Nov 12, 2019

Remove safeRenderCompletion and setImmediate hacks #1197

Closed

slishak mentioned this pull request Jan 13, 2020

[BUG] Custom component stops redrawing when updating from dash 0.41.0 to 0.42.0 plotly/dash#1077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed setState from util #1454

Removed setState from util #1454

mirajp commented Sep 8, 2019

epicfaace left a comment

epicfaace commented Sep 9, 2019

mirajp commented Sep 10, 2019

mirajp commented Sep 11, 2019

epicfaace commented Sep 11, 2019

mirajp commented Sep 11, 2019 •

edited

Loading

chanceaclark commented Sep 11, 2019

epicfaace commented Sep 11, 2019

epicfaace commented Sep 11, 2019

chanceaclark commented Sep 11, 2019

epicfaace Nov 1, 2019

epicfaace commented Nov 1, 2019

	return createComponent(Form, { ...props });
	return createComponent(Form, props);

Removed setState from util #1454

Removed setState from util #1454

Conversation

mirajp commented Sep 8, 2019

Reasons for making this change

Checklist

epicfaace left a comment

Choose a reason for hiding this comment

epicfaace commented Sep 9, 2019

mirajp commented Sep 10, 2019

mirajp commented Sep 11, 2019

epicfaace commented Sep 11, 2019

mirajp commented Sep 11, 2019 • edited Loading

chanceaclark commented Sep 11, 2019

epicfaace commented Sep 11, 2019

epicfaace commented Sep 11, 2019

chanceaclark commented Sep 11, 2019

epicfaace Nov 1, 2019

Choose a reason for hiding this comment

epicfaace commented Nov 1, 2019

mirajp commented Sep 11, 2019 •

edited

Loading