Remove componentWillReceiveProps from Form.jsx #2010

jimmycallin · 2020-08-22T18:04:41Z

Reasons for making this change

componentWillReceiveProps is deprecated, and is currently in use in two places in this project, which causes annoying warnings.

This PR removes the one in Form.jsx, including some refactoring (required due to getDerivedStateFromProps is a static method).

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

jimmycallin · 2020-08-22T18:12:04Z

What makes this refactoring particularly complex is how rjsf handles updates of formData from props and internally. It allows for mixing updates from formData and/or internal state changes, and checks previous formData states to determine whether or not to trigger onChange events. All this would be much easier if the rules was more like:

If props has formData set, do not save formData state internally and let the parent component control formData entirely by listening to onChange events.
If it does not have formData set, save formData state internally and trigger onChange events on changes.

This would greatly simplify the logic without causing any regressions (I think), but I believe this would be a breaking change and would be an overall bigger task.

packages/core/src/components/Form.js

RoboPhred · 2020-08-31T17:06:29Z

packages/core/src/components/Form.js

-      schemaValidationErrors = state.schemaValidationErrors;
-      schemaValidationErrorSchema = state.schemaValidationErrorSchema;
+  static getDerivedStateFromProps(props, state) {
+    if (!deepEquals(state.lastProps, props)) {


Why the reliance on deep equals for comparing property references? Does RJSF really want to support mutating object references? Such an action will not cause react to re-render the component anyway, so supporting this seems to be just a big performance sink.

We are working with very large forms (several hundered fields), and this area has been a pain point for us. I have an outstanding question about the issue here.

Hi @RoboPhred, from my view there is really no reason to use the deepEquals. I didn't spend any thought in reevaluating this, since I just wanted to have a PR with no breaking changes. I tried to replace deepEquals with shallow equals, and it didn't seem to break any tests, so I just pushed this change. We'll see if it passes review. :)

Whoops, that change might not work, as you will always receive a new core props reference each time react re-renders, meaning it will try to make a new state every render. You should compare each individual property to its previous value.

You might be able to get away with something like:

const keys = Object.keys(props); if (keys.some(key => state.lastProps[key] != props[key]) { // regenerate state }

I am a bit worried about adding or removing properties, but I /think/ that if you have an undefined property passed in react, react will actively create that property and set its value as undefined, meaning the property will still show up in Object.keys(). I don't think there are any cases where we might have a property defined in lastProps but not defined in props.

For reference, you can see my previous attempt at this here. In that case, I individually checked the props by name, but I think we can get away with a bulk scan over the new properties.

Ah, good catch. I added an additional check that checked the length of the number of properties, which I think will fix the case of any removed properties not triggering an onChange event. Not sure why the Netlify CI tests are breaking, but I think that's unrelated.

RoboPhred · 2020-08-31T17:08:37Z

If props has formData set, do not save formData state internally and let the parent component control formData entirely by listening to onChange events.

This would go a significant way to fixing this issue, although the continued use of deep equals still needs to be addressed.

epicfaace · 2021-04-14T20:42:56Z

@jimmycallin hey, I think this is a great change! Just haven't had a chance to review it yet. I'll put it on my list and try to get this in soon.

…eiveprops

Zakhar-Kutsko · 2022-02-21T09:48:40Z

Hi there! When you will merge these changes and release a new version?

epicfaace · 2022-02-21T16:27:38Z

It's on the agenda for a sync review in a future meeting https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit

epicfaace

@jimmycallin can you join us at one of our synchronous meetings so you can explain the PR to us? You can add the PR to the agenda of a day in which you're able to make it. Thanks! https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit

epicfaace · 2022-02-25T16:25:12Z

packages/core/src/utils.js

@@ -1265,3 +1265,115 @@ export function schemaRequiresTrueValue(schema) {

  return false;
 }
+
+export function getRegistry(props) {


We should move all of these new functions in utils.js to just Form.js (because they're quite Form-specific)

can we just make them static functions (such as static Registry) within Form? This will make it easier to compare changes in the PR.

packages/core/src/components/Form.js

epicfaace · 2022-02-25T16:50:17Z

packages/core/src/components/Form.js

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

+function handleChange(props, state) {
+  const { lastProps, ...formState } = state;


Figure out -- why do we need lastProps?

On line 62 we need to compare prop changes to determine if the consumer has updated their props.

If the consumer has updated the props, we update the internal state based on the props.

We could previously do this in componentWillReceiveProps since that method supports both viewing previous and new props. But since getDerivedStateFromProps only shows the new prop updates, we need to store the old props in the state.

And the reason why we cannot always derive state from props, is due to us supporting the mixed controlled/uncontrolled behavior we discussed on our last meeting this Friday.

epicfaace · 2022-02-25T16:51:01Z

packages/core/src/utils.js

+}
+
+export function getStateFromProps(props, inputFormData, state = {}) {
+  const edit = typeof inputFormData !== "undefined";


We should figure out -- why does this omit several lines from the original getStateFromProps function?

the lines omitted are mostly just variable declarations that I pick up directly from the prop object instead

epicfaace · 2022-02-25T16:51:24Z

packages/core/src/utils.js

@@ -1265,3 +1265,115 @@ export function schemaRequiresTrueValue(schema) {

  return false;
 }
+
+export function getRegistry(props) {


can we just make them static functions (such as static Registry) within Form? This will make it easier to compare changes in the PR.

…eiveprops

jimmycallin · 2022-02-27T13:11:08Z

@epicfaace i have now updated the PR with all your proposed changes and questions, and merged the lastest from master. so it's ready for another round of review :)

epicfaace

Per our meeting -- deepEquals(props, state.lastProps) is probably too expensive / too much of a performance hit, so let's instead actually change some of the logic so that the component behaves more like a controlled / uncontrolled component.

Maybe we could let the user either

pass in onChange and formData, which makes the Form component fully controlled, or
not do so, which would make the Form component fully uncontrolled.

@jimmycallin , can you write up (or tell us in a future call) how the current behavior of the form works with formdata / onchange? We don't fully understand it.

ArtsiomAntropau · 2022-03-24T20:23:32Z

Hi, @jimmycallin, do you have the time and desire to finish this PR?
Our team can contribute to this PR if it is needed, and you with @epicfaace will highlight what needs to be improved.

jimmycallin · 2022-03-24T21:19:21Z

@ArtsiomAntropau Hi! I have fairly limited time nowadays, and I'm not using rjsf in my work anymore, so if your team can take over this task I would greatly appreciate it.

As @epicfaace mentioned above, this PR in its current state is not a viable solution. We discussed options in a weekly meeting a few weeks ago, and decided that a breaking change where we make the form component a proper controlled/uncontrolled component as described in these two comments makes more sense:

#2010 (comment)
#2010 (review)

If you are willing to implement this, it would be very much appreciated.

cdolek · 2022-03-31T19:54:39Z

Hi guys, is there a middle step where UNSAFE_componentWillReceiveProps is not used anymore, and then the breaking change is introduced with refactoring?

jimmycallin · 2022-04-03T15:03:36Z

Since this isn't a viable approach for removing componentWillReceiveProps, I'll close this one. Due to personal reasons I don't think I will be able to redo the PR according to the previous comment, so if someone else wants to step in, please do so.

Kos · 2022-04-04T10:40:42Z

Is there a relevant issue which we could follow for updates regarding this bit?

We discussed options in a #2677 a few weeks ago, and decided that a breaking change where we make the form component a proper controlled/uncontrolled component as described in these two comments makes more sense

Zakhar-Kutsko · 2022-04-05T09:15:49Z

Hi there! Following the documentation, by default, <Form /> is already an uncontrolled component.@epicfaace, Or is your suggestion exactly to remove deepEquals(props, state.lastProps)?

cdolek · 2022-04-07T14:39:01Z

@ArtsiomAntropau is your team working on this?

ArtsiomAntropau · 2022-04-07T14:42:44Z

@cdolek Hi,
@Zakhar-Kutsko still investigating on it

summer-cook · 2023-01-25T17:34:36Z

Hi @ArtsiomAntropau , was just wondering if there is an open issue for this I could follow? Still getting this warning on our Form

refactor: form.jsx is no longer using componentwillreceiveprops

a9d8e05

jimmycallin force-pushed the no-componentwillreceiveprops branch from f2d6f37 to a9d8e05 Compare August 23, 2020 15:49

sumanbh reviewed Aug 25, 2020

View reviewed changes

packages/core/src/components/Form.js Show resolved Hide resolved

return null from getDerivedStateFromProps as default

2d35019

RoboPhred reviewed Aug 31, 2020

View reviewed changes

Jimmy Callin added 2 commits August 31, 2020 22:18

change deepEquals to shallow equals in getDerivedStateFromProps

33aee98

checking each key for shallow changes in getDerivedStateFromProps

4532e45

jimmycallin mentioned this pull request Mar 22, 2021

ANNOUNCEMENT: Breaking changes planned for v3 #2297

Closed

5 tasks

epicfaace self-assigned this Apr 14, 2021

epicfaace added the p1 Important to fix soon label Apr 14, 2021

Merge remote-tracking branch 'origin/master' into no-componentwillrec…

f26788b

…eiveprops

jimmycallin mentioned this pull request Apr 17, 2021

remove componentWillReceiveProps from altdatewidget #2339

Merged

7 tasks

Jimmy Callin and others added 3 commits April 17, 2021 23:49

Merge branch 'master' into no-componentwillreceiveprops

28c697c

Merge branch 'master' into no-componentwillreceiveprops

092aab0

Merge branch 'master' into no-componentwillreceiveprops

ed89981

epicfaace mentioned this pull request May 30, 2021

State Change in onSubmit Breaks Validation #2408

Closed

3 tasks

jimmycallin mentioned this pull request Jun 25, 2021

ANNOUNCEMENT: Breaking changes planned for v4 #2437

Closed

3 tasks

epicfaace mentioned this pull request Jul 18, 2021

Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended #1794

Closed

Kos mentioned this pull request Aug 3, 2021

Bug with setState in event handlers #1023

Closed

2 tasks

epicfaace reviewed Feb 25, 2022

View reviewed changes

jimmycallin added 3 commits February 27, 2022 13:36

Merge remote-tracking branch 'origin/master' into no-componentwillrec…

181f0da

…eiveprops

Revert change for deepEquals function

a40ce16

moved util functions back as static functions to Form

ed618c9

epicfaace reviewed Mar 4, 2022

View reviewed changes

jimmycallin closed this Apr 3, 2022

epicfaace mentioned this pull request Apr 8, 2022

Calling onChange on the Form's parent due to props no longer throws an error #2796

Closed

8 tasks

summer-cook mentioned this pull request Jan 25, 2023

SPIKE: research getDerivedStateFromProps scientist-softserv/webstore#118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove componentWillReceiveProps from Form.jsx #2010

Remove componentWillReceiveProps from Form.jsx #2010

jimmycallin commented Aug 22, 2020 •

edited

Loading

jimmycallin commented Aug 22, 2020

RoboPhred Aug 31, 2020 •

edited

Loading

jimmycallin Aug 31, 2020

RoboPhred Aug 31, 2020

jimmycallin Aug 31, 2020

RoboPhred commented Aug 31, 2020

epicfaace commented Apr 14, 2021

Zakhar-Kutsko commented Feb 21, 2022

epicfaace commented Feb 21, 2022

epicfaace left a comment

epicfaace Feb 25, 2022

epicfaace Feb 25, 2022

epicfaace Feb 25, 2022

jimmycallin Feb 27, 2022

jimmycallin Feb 27, 2022

epicfaace Feb 25, 2022

jimmycallin Feb 27, 2022

epicfaace Feb 25, 2022

jimmycallin commented Feb 27, 2022

epicfaace left a comment

ArtsiomAntropau commented Mar 24, 2022

jimmycallin commented Mar 24, 2022

cdolek commented Mar 31, 2022 •

edited

Loading

jimmycallin commented Apr 3, 2022

Kos commented Apr 4, 2022

Zakhar-Kutsko commented Apr 5, 2022 •

edited

Loading

cdolek commented Apr 7, 2022

ArtsiomAntropau commented Apr 7, 2022

summer-cook commented Jan 25, 2023

Remove componentWillReceiveProps from Form.jsx #2010

Remove componentWillReceiveProps from Form.jsx #2010

Conversation

jimmycallin commented Aug 22, 2020 • edited Loading

Reasons for making this change

Checklist

jimmycallin commented Aug 22, 2020

RoboPhred Aug 31, 2020 • edited Loading

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

RoboPhred commented Aug 31, 2020

epicfaace commented Apr 14, 2021

Zakhar-Kutsko commented Feb 21, 2022

epicfaace commented Feb 21, 2022

epicfaace 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

jimmycallin commented Feb 27, 2022

epicfaace left a comment

Choose a reason for hiding this comment

ArtsiomAntropau commented Mar 24, 2022

jimmycallin commented Mar 24, 2022

cdolek commented Mar 31, 2022 • edited Loading

jimmycallin commented Apr 3, 2022

Kos commented Apr 4, 2022

Zakhar-Kutsko commented Apr 5, 2022 • edited Loading

cdolek commented Apr 7, 2022

ArtsiomAntropau commented Apr 7, 2022

summer-cook commented Jan 25, 2023

jimmycallin commented Aug 22, 2020 •

edited

Loading

RoboPhred Aug 31, 2020 •

edited

Loading

cdolek commented Mar 31, 2022 •

edited

Loading

Zakhar-Kutsko commented Apr 5, 2022 •

edited

Loading