preserve the form data so when we update the additionalErrors the dat… #2478
Conversation
…a is not reset to the initial state
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@sdirix please review, to check the issue in the current master branch just apply the change for ExampleView.vue and then try to test the additional-errors example, e.g. change the form once it is loaded and then click on the "Add Additonal Error" button and observe that the form data is reset, then check the https://deploy-preview-2478--jsonforms-examples.netlify.app/vue-vuetify/#additional-errors |
|
So the error occurs only if the user of JSON Forms:
I don't think that this is a valid use case. Either the props of the JSON Forms component are "frozen" and the management is done by JSON Forms, or the props are always live. The error case is a weird mix which is not really valid. The same issue will occur in Angular and React if only parts of the props are updated. Either they should never be updated by the user, i.e. uncontrolled variant, or they should all be updated, i.e. controlled variant. |
|
I understand the distinction between controlled and uncontrolled usage, but in practice consumers of JSON Forms may mix these patterns—intentionally or unintentionally—especially in larger applications with multiple state sources. Even if this “weird mix” is not the recommended pattern, supporting it makes JSON Forms more fault-tolerant and easier to integrate in real-world scenarios where the state flow is not always perfectly aligned with controlled/uncontrolled paradigms. |
…the data is changed
…imple type like number, string, array and etc.
|
@sdirix @lucas-koehler please also check the update where the generated schema and uischema are properly regenerated when the data is changes - please check this example dynamic when if you change the data to a number and save then the ui will be updated accordingly - in previous versions that was not true. |
…to non null uischema in JsonForms
|
@sdirix please review |
lucas-koehler
force-pushed
the
master
branch
4 times, most recently
from
24c8cb2 to
d86047e
Compare
|
@sdirix @lucas-koehler please review |
lucas-koehler
left a comment
lucas-koehler
left a comment
There was a problem hiding this comment.
Hi @kchobantonov ,
Thanks for the updates! I have some comments inline unrelated to the setting of this.dataToUse = newEvent.data;.
I am still a bit concerned about this:
- It makes the data propagation less intuitive:
- In the controlled case, the data is set to dataToUse again after it has already been updated. Granted, this does not trigger an additional invocation of
coreDataToUpdate(). - In the uncontrolled case, this triggers
coreDataToUpdatean additional time without it being needed. This has a possible performance penalty because it invokes the middleware again - In the mixed case, it triggers the
coreDataToUpdatelike in the uncontrolled case. From a data point of view that should also not be necessary because the data is uncontrolled. Granted, this is acceptable considering the issues that this tries to fix.
- In the controlled case, the data is set to dataToUse again after it has already been updated. Granted, this does not trigger an additional invocation of
As this does make the state handling less intuitive and lowers performance in uncontrolled mode as far as I can tell, I am leaning towards not introducing supporting this mixed case.
Do you have a concrete use case in mind where this is necessary? For a diffuse robustness increase, I prefer to not introduce this because consumers can always control the data in the component rendering the form. Even if the state handling is more complex for other properties.
We can of course add this to the documentation though.
| }, | ||
| data(newData) { | ||
| this.dataToUse = newData; | ||
| this.dataToUse = newData === undefined ? {} : newData; |
There was a problem hiding this comment.
Why is this necessary? For instance, we don't do this in React
| const createDefaultLayout = (): Layout => ({ | ||
| type: 'VerticalLayout', | ||
| elements: [], | ||
| }); | ||
| const generateUISchema = (schema: JsonSchema) => | ||
| Generate.uiSchema(schema, undefined, undefined, schema) ?? | ||
| createDefaultLayout(); |
There was a problem hiding this comment.
This is behavior that shouldn't be defined specifically here. If the Generator does not handle this already, we might fix it there for all.
| monaco.Uri.parse(toDataUri(props.example.name)), | ||
| event.data !== undefined ? JSON.stringify(event.data, null, 2) : '', | ||
| ); | ||
| state.data = event.data; |
There was a problem hiding this comment.
I get that this is introduced to test the new behavior but even if we were to merge it, I don't think the example app use this unrecommended mixed approach. Thus, this should be reverted
4 participants
…a is not reset to the initial state