React-jsonschema-form: Remove safeRenderCompletion and setImmediate hacks

Created on 26 Feb 2019  ·  5Comments  ·  Source: rjsf-team/react-jsonschema-form

This is a proposal for V2 project, where's the chance to get rid of setImmediate hacks regarding setState:

  • replace calls to custom setState wrapper (which uses setImmediate hack) with proper React setState calls and with callback where appropriate, and eliminate setState wrapper.
  • remove safeRenderCompletion
  • remove any other setImmediate hacks (at least outside of testing code)

The code to be removed makes assumptions about React behaviour and JS engine performance which don't work as they changed (and will change) over time.

See discussion at #891 (older: #562)

Related currently open issues: #1164, #1023 (maybe), #544, #513 (maybe), #446 I'm sure there are many other (potentially obscure) errors, mostly race-conditions, coming from this hack. See for example my PR #1068.

There's the possibility that bugs are raised or performance is degraded by that change, but these problems should always be fixable without a false setImmediate "fix".

P.S: nice to see that the project gains traction again, thanks for all working on it. We're happy to contribute!

breaking change enhancement performance

Most helpful comment

After just spending three hours debugging why some keystrokes were skipped when typing rapidly in our complex form and then discovering the need for safeRenderCompletion={true}, big 👍 for removing these hacks.

All 5 comments

Thanks for making this issue. Do you know why exactly the custom setState wrapper with setImmediate was added in the first place? Will removing this necessarily be a backwards incompatible change that would require a v2?

https://github.com/mozilla-services/react-jsonschema-form/blob/f9d4c63cac24146e522528d33eb04f465464e052/src/utils.js#L786

No, I don't know why.

Some forensics:

  • the setState wrapper was introduced in 6159cb4834a082b2af2154e6f978b9ad57e96d51, but this mostly refactored existing setImmediate hacks into this wrapper to enable central circumvention of setImmediate via safeRenderCompletion.
  • before that, many setImmediate calls (all of them?) were introduced in PR #153 "Improved async rendering strategy" by @n1k0

If I understand everything correctly:

  1. status quo: setState was used properly, with onChange within callback (so no problems with correctness, race conditions)
  2. it became clear that react-jsonschema-form is too slow: many components are stateless and there is no memoization. See #147. The examples linked there demonstrating the performance degradation are still working...
  3. first try to fix it was PR #152, using memoization (interestingly, the PR also introduces some setImmediate calls)
  4. it seems that during implementation of PR #152, using setImmediate showed so much promise that PR #152 was aborted in favor of PR #153

Regarding backwards compatibility:

In theory, after eliminating setState wrapper and setImmediate and after fixing the performance problems, everything should still just work™. But most definitely, some behavior will change slightly, people are relying on timings, and so on. So I would consider this a breaking change.

This is even more true if we take new React hooks into consideration, which could be a solution for the performance problem, since this requires upgrading the peer dependency on React to 16.8.0.

After just spending three hours debugging why some keystrokes were skipped when typing rapidly in our complex form and then discovering the need for safeRenderCompletion={true}, big 👍 for removing these hacks.

Thanks for digging into this. I never really understood the purpose of those hacks myself. I'd love to see them gone.

This is nearly fixed by #1454, but we still have a few setImmediate hacks in the playground and tests that need to be removed: https://github.com/rjsf-team/react-jsonschema-form/search?q=setImmediate&unscoped_q=setImmediate

Was this page helpful?
0 / 5 - 0 ratings

Related issues

epicfaace picture epicfaace  ·  3Comments

norim13 picture norim13  ·  3Comments

pablen picture pablen  ·  3Comments

FBurner picture FBurner  ·  3Comments

ClockerZadq picture ClockerZadq  ·  3Comments