Skip to content
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.

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

Add a note about needing safeRenderCompletion and slow CPU:s #544

Closed
wants to merge 1 commit into from

Conversation

matias-sandell
Copy link

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

Reasons for making this change

See the discussion in #446.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added

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

As discussed in rjsf-team#446.
Copy link
Contributor

@glasserc glasserc left a comment

Choose a reason for hiding this comment

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

Thanks for your effort, and your patience since I'm only getting to this now 😳. I'm still quite new to the project so I'm not comfortable merging this without a clearer understanding of what safeRenderCompletion is and why it isn't the default (which I'll try to get from @n1k0). I've left a couple comments in case you are more comfortable with the architecture than I am :)

> Note: Prior to v0.35.0, the `options` prop contained the list of options (`label` and `value`) for `enum` fields. Since v0.35.0, it now exposes this list as the `enumOptions` property within the `options` object.
> Notes:
> * Prior to v0.35.0, the `options` prop contained the list of options (`label` and `value`) for `enum` fields. Since v0.35.0, it now exposes this list as the `enumOptions` property within the `options` object.
> * If your custom widget only calls `onChange` when the user is blurring the field (maybe to keep RJSF from updating the model for every key stroke) there is an edge case where users on devices with less processing power could potentially submit stale `formData` when focused in an input field while submitting the form due to [RJSF util setState](https://github.com/mozilla-services/react-jsonschema-form/blob/master/src/utils.js#L495) not using the React `setState` callback unless we set the `safeRenderCompletion` prop to `true` on the `<Form>`component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is one complicated sentence. I'd like to break it up into smaller, more digestible chunks. I understand that there's a lot you want to convey -- there's an edge case, it happens in these circumstances, this is why it happens, and here's the workaround -- but I think each of those could even be a whole sentence. Maybe we should start with the symptom (submitting with stale data?) since that's the thing someone might notice?

I just noticed that we don't actually document safeRenderCompletion anywhere, which makes me think it's a hack used for tests. But because it's a prop of the form, rather than any specific widget, I wonder if we should document it somewhere more global instead?

@n1k0
Copy link
Collaborator

n1k0 commented Apr 15, 2017

safeRenderCompletion is only used for tests to ease writing them, where we need to ensure a component is fully rendered before performing assertions on the resulting DOM state. I never documented this option as I could never see any use case in real world webapps, as it slows execution a lot and makes the UI barely usable.

@matias-sandell
Copy link
Author

The funny thing is, we went for only updating the formData when blurring the input since the RJSF + Material-UI combo was so slow when updating on every key stroke. That in turn causes the stale data issue I tried to describe above to appear (and solved by setting safeRenderCompletion).

But I believe that's more of a Material-UI problem which hopefully will be solved when they're done with their rewrite so I'm fine with this not getting merged or documented.

@numbbbbb
Copy link

numbbbbb commented Apr 19, 2018

I waste 3 hours to figure out why my onChange function doesn't give me the changed data. I understand you may have some reason to add this config option, but you really shouldn't set it to false by default and SAY NOTHING ABOUT IT IN YOUR README.

@epicfaace
Copy link
Member

Closing this because we no longer use safeRenderCompletion.

@epicfaace epicfaace closed this Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants