Data loss when using React controlled style

[jwueller]

Describe the bug

<JsonForms/> seems to do asynchronous onChange, which breaks controlled style.

Expected behavior

onChange is synchronous, thereby allowing use in controlled style.

Steps to reproduce the issue

Here is a very simple reproduction:

const schema = {
  type: "object",
  properties: {
    value: {type: "number"},
  },
};

function App() {
  const [data, setData] = useState({value: 0});

  useEffect(() => {
    const interval = setInterval(() => {
      // This is a stand-in for some external data change.
      setData(data => ({...data}));
    }, 10);

    return () => clearInterval(interval);
  }, []);

  return (
    <JsonForms 
      data={data}
      renderers={vanillaRenderers} 
      cells={vanillaCells}
      schema={schema}
      onChange={({data}) => setData(data)}/>
  );
}

https://codesandbox.io/p/sandbox/intelligent-snyder-y2p7m4?file=%2Fsrc%2FApp.tsx%3A18%2C10

You can easily see that data is being lost when rapidly typing a few digits into the input field.

Screenshots

No response

In which browser are you experiencing the issue?

Any

Which Version of JSON Forms are you using?

v3.1.0

Framework

React

RendererSet

Vanilla

I didn't test other RendererSets, but I assume they would have the same issue in React.

Additional context

It seems that the problem might be caused, at least in part, by the debounce in JsonFormsContext.tsx, which, at first glance, seems to be fundamentally incompatible with controlled style.

[jwueller]

I'm happy to attempt a pull request if the underlying issue can be identified by someone with better knowledge of the codebase, since I don't fully understand the reasoning for adding the debounce in the first place.

[sdirix]

Hi @jwueller,

I answered in more detail in the community thread.

To summarize:

• The design of the JsonForms component is not ideal as you noticed. It would be nicer to separate cleanly between controlled and uncontrolled use cases which would then avoid issues like you described.

• A typical workaround for the described issue is to only modify the data handed over to JSON Forms when the user is currently not editing the form.

We would happily accept a contribution to improve the design. We're also working on other JSON Forms 4.0 changes so this would be a good fit for a release.

[dchambers]

That design smell is also why there is a small debounce in the JSON Forms component: The forms auto-fill mechanism of Chrome entered data so fast that JSON Forms was stuck in an endless rerender loop.

Hi @sdirix 👋. We've recently started using JSON Forms (thank you for this lib BTW 🙇) and are hitting an issue where some of our UI react-testing-library tests are unreliable (the first letter typed into an input sometimes goes missing) as a consequence of this debounce; where the problem goes away if we update the local copy of @jsonforms/react to stop debouncing updates, and to instead update synchronously.

Do you know if this is an issue that others have also encountered?

[dchambers]

In case it helps anyone else, I was able to mitigate this issue within our code by interleaving sleep statements between updates to input fields, so for example this test code:

await userEvent.type(screen.getByLabelText(streetLabel), 'High Street');
await userEvent.type(screen.getByLabelText(streetNumberLabel), '11');
await userEvent.type(screen.getByLabelText(cityLabel), 'London');
await userEvent.type(screen.getByLabelText(postalCodeLabel), 'NE1 1AA');

is made reliable by changing to this:

await userEvent.type(screen.getByLabelText(streetLabel), 'High Street');
await sleep(5);
await userEvent.type(screen.getByLabelText(streetNumberLabel), '11');
await sleep(5);
await userEvent.type(screen.getByLabelText(cityLabel), 'London');
await sleep(5);
await userEvent.type(screen.getByLabelText(postalCodeLabel), 'NE1 1AA');

where sleep is defined as:

const sleep = (ms: number) => new Promise(resolve => {
    setTimeout(resolve, ms)
})

[sdirix]

That design smell is also why there is a small debounce in the JSON Forms component: The forms auto-fill mechanism of Chrome entered data so fast that JSON Forms was stuck in an endless rerender loop.

Hi @sdirix 👋. We've recently started using JSON Forms (thank you for this lib BTW 🙇) and are hitting an issue where some of our UI react-testing-library tests are unreliable (the first letter typed into an input sometimes goes missing) as a consequence of this debounce; where the problem goes away if we update the local copy of @jsonforms/react to stop debouncing updates, and to instead update synchronously.

Do you know if this is an issue that others have also encountered?

We have a lot of UI tests too but we did not encounter these issues there. However we're still using enzyme, so some behavior might be different to react-testing-library.

In case it helps anyone else, I was able to mitigate this issue within our code by interleaving sleep statements between updates to input fields, so for example this test code:

await userEvent.type(screen.getByLabelText(streetLabel), 'High Street');
await userEvent.type(screen.getByLabelText(streetNumberLabel), '11');
await userEvent.type(screen.getByLabelText(cityLabel), 'London');
await userEvent.type(screen.getByLabelText(postalCodeLabel), 'NE1 1AA');

is made reliable by changing to this:

await userEvent.type(screen.getByLabelText(streetLabel), 'High Street');
await sleep(5);
await userEvent.type(screen.getByLabelText(streetNumberLabel), '11');
await sleep(5);
await userEvent.type(screen.getByLabelText(cityLabel), 'London');
await sleep(5);
await userEvent.type(screen.getByLabelText(postalCodeLabel), 'NE1 1AA');

where sleep is defined as:

const sleep = (ms: number) => new Promise(resolve => {
    setTimeout(resolve, ms)
})

Thanks for posting this workaround which might help other developers. We also plan to switch testing frameworks in the "nearish" future, so we will definitely have a look at the issue then.

[dchambers]

Thanks for the update @sdirix 🙇.

As for ourselves, we just encountered a more severe version of this race condition (that can't be mitigated with sleep commands) after performing an npm update as part of work to upgrade past a vulnerable library. In this new incarnation of the race condition we're seeing letters get lost in the middle of text entered with a single userEvent.type invocation.

Perhaps we could help by offering a PR to move to the cleaner architecture that you describe? In my mind this would be as simple as:

1. Treating data as initialData (and ignoring updates and preserving internal state) in the case that no onChange handler is provided.
2. Not maintaining any internal state, and leaving the state to be injected through the data and error props, otherwise.

I wasn't sure from your post whether you were thinking exactly the same however, since you didn't mention how you would switch between the controlled and uncontrolled modes, and whether this might even be done by having variations of the JsonForms component for users to select between?

Thanks again, Dominic.

[sdirix]

Hi @dchambers, that would be amazing! I'll come back with more details next week.

[dchambers]

Hi @sdirix 👋, just a reminder that we're ready to start work on a PR once you're happy with the approach that we'll be taking.

[sdirix]

Hi @dchambers, sorry for the late reply.

Treating data as initialData (and ignoring updates and preserving internal state) in the case that no onChange handler is provided.

This will not work. The onChange handler is handed over by every user of JSON Forms as no matter whether it's controlled or uncontrolled, you need some way to access the data and potentially errors. Or maybe I did misunderstand something?

---

A simple change is to offer data and initialData props.

• If initialData is used, then JSON Forms is in "uncontrolled mode", i.e. it behaves as it does today but ignores any changes to initialData or data
• If initialData is not used, then JSON Forms is in "controlled mode". Here we customize the core reducer to override the "update data" handling. We still perform the action but do not update the internal state and instead emit the new data and errors to the outside world.

Conceptually this works, however it's not ideal for backward compatibility as it will break every user who did not update the data after handing it over initially.

A backward compatible change would be the introduction of the new data props and keeping the behavior of the current one. Or we allow to additionally configure a "mode" which is either "controlled", "uncontrolled" or "mixed" with "mixed" being the default.

@lucas-koehler What do you think?

[dchambers]

A backward compatible change would be the introduction of the new data props and keeping the behavior of the current one. Or we allow to additionally configure a "mode" which is either "controlled", "uncontrolled" or "mixed" with "mixed" being the default.

Very nice 👍.

A small additional tweak would be to only use mode to control the behaviour for developers using data rather than initialData, in which case its value would only need to switch between mixed or controlled (with mixed being the default).

This would be nice since:

1. As before, the change would be backwardly compatible so it could be released immediately.
2. But now, additionally, the default could be safely changed from mixed to controlled on the next major release without creating issues for code using initialData.

Thoughts?

[sdirix]

Hi @dchambers,

We discussed this issue internally and came to the following conclusions:

• We would like to go with a mode solution instead of introducing additional data props like initialData
• Supported modes should be at least mixed and controlled.
  • Optionally we could also support uncontrolled but that is not that important as it can also be achieved with mixed
• We would like to use the opportunity to refactor the behavior of config:
  • Over time the use of config developed to only serve as "default UI Schema options", however it should serve as the place for all generic JSON Forms options
  • Therefore the new mode prop should be placed in config.
  • Existing users of config should move their options into the new defaultUiSchemaOptions field of config
• To be backward compatible we will check the content of config whether it has defaultUiSchemaOptions defined. If yes, we will treat is as the new config. If not, we will treat the whole object as defaultUiSchemaOptions as it was before
• To transition users we will type the interface of the config to have the defaultUiSchemaOption mandatory, so they will get complains in Typescript when updating. Still their code will continue to work, for example if they are using pure Javascript.

Does this make sense to you? Would you like to contribute this change?

[dchambers]

Does this make sense to you? Would you like to contribute this change?

Hi @sdirix,

Yes, this all makes sense, and we'd be very happy to contribute a PR for this change.

One minor issue that we'd like to see plugged however is that, although it would be a backwardly compatible change for JS consumers, it would be a breaking change for TS consumers. Additionally, the TS code that fixed up the potentially incorrect type would be harder to write because it would be wanting to handle a type case that the developer was aware of, but the existence of which hadn't been admitted to the compiler.

Instead, can we propose a very minor tweak to the plan as given. Assuming the following types:

type GivenConfig = Config | DefaultUiSchemaOptions; // only used once at the input boundary

type Config = {
  mode: 'controlled' | 'uncontrolled' | 'mixed';
  defaultUiSchemaOptions: DefaultUiSchemaOptions;
};

type DefaultUiSchemaOptions = {
  restrict: boolean;
  trim: boolean;
  showUnfocusedDescription: boolean;
  hideRequiredAsterisk: boolean;
};

then:

1. Config is the type that's used throughout the codebase.
2. GivenConfig is a type that's only used in one place, at the input boundary where the given config is received, and where that given config is immediately converted to type Config for consumption in all other parts of the codebase.

With this small tweak:

1. The update will be backwardly compatible for TS users too.
2. The code that needs to process the given config won't need to perform any type chicanery to do so.
3. All other code throughout the code base will be able to use the pristine Config type which we want it to use.
4. Support for GivenConfig can be dropped using a tiny change in a single location as part of a future 4.0 release.

Assuming that you're happy with this slight amendment we'll look to create a WiP PR for you to review in due course?

[sdirix]

Hi @dchambers,

Sadly the suggestion is actually backward incompatible instead of just producing a typing issue. Not directly in the JSON Forms API, but within custom renderers. They will be broken as they expect the injected config to look like the one handed over to JSON Forms and not to be adapted.

Regarding the suggested types: They look mostly fine, however almost all properties need to be optional. Also the DefaultUiSchemaOptions need to allow arbitrary additional properties as many users hand in additional custom attributes.

Another point to think about: We already have a backward incompatible change in the works which will require a major version update. So we could combine this change with the other one for the release. In that case the suggested breakage of config in custom renderers could be considered okay.

Of course we could also "deprecate" config or introduce another prop for the config, e.g. options to avoid breakages at the cost of additional props and more confusion for the developer what is used for what.

If you like you can definitely already start with the developing of the main feature (controlled mode) itself. The way we expose it at the end is not really important for now during development.

[dchambers]

Hi @sdirix,

Thanks so much for your prompt reply. Everything you said makes perfect sense. Taking all of that into consideration, I believe that these updated types would work:

type DefaultUiSchemaOptions = {
    restrict?: boolean;
    trim?: boolean;
    showUnfocusedDescription?: boolean;
    hideRequiredAsterisk?: boolean;
} & Record<string, unknown>;

type NonOptionConfig = {
    mode: 'controlled' | 'uncontrolled' | 'mixed';
};

// the main `Config` type for use throughout the codebase
type Config = NonOptionConfig & {
    defaultUiSchemaOptions: DefaultUiSchemaOptions;
};

// the type accepted at the input boundary so we remain backwardly compatible
type GivenConfig = Config | DefaultUiSchemaOptions;

// the type given to renderers, since requiring custom renderers to accept `Config` would be a breaking change
type RendererConfig = DefaultUiSchemaOptions & { nonOptionConfig: NonOptionConfig };

where GivenConfig and RendererConfig could be dropped as part of a future breaking release (e.g. v4, or v5, etc).

We have started development BTW 😄, but feel free to holla if you think I've still got this wrong!