WIP: introduce controlled & uncontrolled states for data/onChange management

[dholmes2409]

Resolves #2174

• Introduces 2 new modes which can be configured via a new 'mode' prop
• Missing tests as pending review from JsonForms team to be sure we're on the right track
• Missing configuration for CustomRenders as still pending discussion on Data loss when using React controlled style #2174

[netlify]

✅ Deploy Preview for jsonforms-examples ready!

Name	Link
🔨 Latest commit	77624c6
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/6560a23933cfe1000846d61d
😎 Deploy Preview	https://deploy-preview-2196--jsonforms-examples.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[CLAassistant]

All committers have signed the CLA.

[sdirix]

Thanks for the contribution ❤️ ! I'll take a detailed look once I have some cycles available, probably at the end of the week / start of next week.

[sdirix]

I had a look at the PR already. I'm very intrigued by the suggestion to return a user input in the customized dispatch. This basically introduces some kind of middleware support. Thinking about it, I think this use case can also be solved by actually introducing generic middleware support. I fleshed this out a bit in a comment. Let me know what you think about it.

[sdirix]

We should probably put the StateProvider in useState and issue a warning in case a developer tries to switch the mode after the fact.

[sdirix]

Is there a benefit of introducing this additional useCallback? Later where we use onChangeHandler we could just use onChangeRef.current? directly.

[sdirix]

This was copied from the previous code and not introduced by you, but actually we don't need the useEffect here. We can simply set the onChangeRef.current directly here without the effect.

[sdirix]

Ah interesting, this is not only emitting the data but actually is some kind of middleware support, only for the jsonforms/UPDATE case. However the user has no chance to actually return something sensible in the onChangeHandler here. To not update the internal state we need to return the core here, but the user does not have access to it.

Hmm thinking about it, we could actually just introduce a generic middleware support and then this use case of controlled vs uncontrolled mode would also be covered.

This could look like this:

const middleware = props.middleware ?? (core, action, coreDispatch) => coreDispatch(core, action);

const actualDispatch = (action: CoreActions) => {
  return middleware(core, action, coreDispatch);
}

The middleware for this case could then look like this:

const controlledModeMiddleware = (state, action, defaultDispatch) => {
  if(action.type === 'jsonforms/UPDATE'){
     const updatedState = defaultDispatch(state, action);
     myChangeHandler(data: updatedState.data, errors: updatedState.errors);
     return state;
   } else {
     return defaultDispatch(state, action);
   }
}

The new config could then be used to disable the default (debounced) emitter. Although not even that is needed as the onChange handler does not need to be handed over in that case.

[dchambers]

Hi @sdirix 👋.

As I mentioned on a couple of my PR review comments, there's a lot of repetition between the different state providers, and one piece of mode related logic is not currently encapsulated within the relevant state provider, where it would ideally reside.

I just performed some diffs between the mixed, controlled and uncontrolled state providers to see what the lie of the land was, and this is what I determined:

1. Mixed debounces onChange.
2. Uncontrolled passes core.data to coreDispatch(Actions.updateCore), whereas Mixed and Controlled pass data.
3. Controlled returns a wrapped dispatch function rather than the underlying dispatch function.
4. Controlled 'effectively' passes a dummy onChange:
   1. This is currently implemented within JsonFormsDataProvider, which only invokes onChange if mode !== 'controlled', but it makes more sense that all mode logic be centralised within the data provider.

This means that there are three points of variance that any middleware (or data-provider) would have to account for:

1. Whether the internally or externally sourced data is passed to the reducer.
2. Whether onChange is wrapped.
3. Whether dispatch is wrapped.

As I understand it, the middleware idea would only help with the last of these three, whereas the data provider can handle all three.

Although the state providers currently have a lot of repetition between them, I've taken a look and this can be dramatically reduced by creating a couple of shared hooks (e.g. useCoreDispatcher and useCoreUpdater) to encapsulate the common blocks of code. If we also take the opportunity to move the state providers into their own files, then that would clean up things even further.

Let me know if you agree or not, and I (or @dholmes2409) can take a look at getting the PR updated as described.

[sdirix]

Hi @dchambers, thanks for the summary! I thought we could efficiently tackle the "mixed" vs "controlled" case by only implementing the suggested middleware approach without the need to introduce multiple state providers. However I might have also overlooked some edge case / use case. If only "mixed" and "controlled" is offered, would this be sufficient for your use cases, or would you specifically require all three modes?

I will take a more detailed look at your analysis and my suggestion early next week and then come back to you with the go ahead for the middleware or the different state providers. Overall planning wise I'm pretty confident that we will get this into a November release.

[dchambers]

Hi @dchambers, thanks for the summary! I thought we could efficiently tackle the "mixed" vs "controlled" case by only implementing the suggested middleware approach without the need to introduce multiple state providers. However I might have also overlooked some edge case / use case. If only "mixed" and "controlled" is offered, would this be sufficient for your use cases, or would you specifically require all three modes?

Hi @sdirix 👋, I hope you had a good weekend. "controlled" is the only mode we'd really want to use, so this would definitely be sufficient for us if it can be made to work. That said, I'm not currently clear as to how a middleware could replace the onChange that ends up being passed to JsonFormsDataProvider, if you're able to provide some clarity here?

[sdirix]

Hi @dchambers!

I quickly implemented my suggestion. However I did not yet test nor compile it. You can find it in my fork, see here.

Please have a look. Using a middleware you no longer need to hand over a "onChange" as you get the latest updates anyway. Therefore the debounce is also avoided.

[sdirix]

Hi @dchambers,

I didn't add the controlledMiddleware to the repository as I expect it to be implemented in JSON Forms consumers. If more people have this use case we could maybe offer a factory util to create such a middleware conveniently.

Based on your example I think this can look like this:

/**
 * Controlled variant of JSON Forms.
 * Props are only read in first render pass. Updates to them are ignored.
 */
const ControlledJsonForms = ({ schema, initialData }) => {
  const [formState, setFormState] = useState<{
    data: any;
    errors: ErrorObject[];
  }>({ data: initialData, errors: [] });

  const validator = useMemo(() => createAjv().compile(schema), []);

  const controlledMiddleware: Middleware = (state, action, reducer) => {
    if (action.type === 'jsonforms/UPDATE') {
      const newState = reducer(state, action);
      validator(newState.data);
      setFormState({ data: newState.data, errors: validator.errors || [] });
      return state; // this will lead to JSON Forms ignoring the change within itself
    } else {
      return reducer(state, action);
    }
  };

  return (
    <JsonForms
      schema={schema}
      data={formState.data}
      additionalErrors={formState.errors}
      validationMode='NoValidation'
      middleware={controlledMiddleware}
      renderers={materialRenderers}
      cells={materialCells}
    />
  );
};

The controlledMiddeware could be placed in a useCallback but it does not have to be.

Overall this looks like I expected, except for the validation part. I realized that if the validation shall also be controlled then it either must be performed outside, like I did now, or it would require more code within the controlledMiddleware delving deeper into the internals. As there is no downside in doing the validation on our own I preferred that approach.

What do you think, does this cover your use case?

[sdirix]

@dchambers Does this work for you? If yes, then I would like to get it in ideally before the next release.

[dholmes2409]

Hey @sdirix, I had a play around with this today and it seems to cover our use case. We would need to tweak our usage as we pass a custom onChange handler but we could easily move the logic into the controlledMiddleware.
Should I close this PR and we go with the approach you committed to your fork?

[sdirix]

To get this in faster it would be nice to have some tests. Would you like to contribute them? You could force push this PR and use my commit as basis. In the tests you could add the controlledMiddleware and verify that your use case works as expected. Then it's ensured that it will not be broken in the future ;)

If you don't have time then that's also fine, we are definitely eager to get this feature in, so we will eventually also work on this. However as always I can't give any promises regarding a due date as this depends on our work load.

[dholmes2409]

Moved this into PR #2220

[dchambers]

LGTM! Some comments for you however.

[dchambers]

Rename to JsonFormsDataProviderProps?

[dchambers]

Revert to using lambda syntax?

[dchambers]

Maybe something you'll come back to as the PR is refined, but might there be an opportunity to reduce the amount of repeated code between JsonFormsMixedStateProvider, JsonFormsControlledStateProvider and JsonFormsUncontrolledStateProvider (e.g. either by having some state providers be defined in terms of other state providers, by having the providers share re-usable blocks)?

[dchambers]

Having mode checking logic still be needed in JsonFormsDataProvider (and not be in the state providers) may be indicative of an opportunity to improve things further.