Closed MarekBodingerBA closed 2 weeks ago
Also consider adding test for all the modified functions to ensure the param is passed through properly? I know that's a bunch more work, so if you can justify why it is unnecessary, I'll listen.
@heath-freenome I definitely agree that history of this screams that it needs tests.
In the short term, as it acts broken now (the custom merger is called let's say in 50% cases in real world application), I think there are two options: completely revert the feature (which I don't prefer), or merge this fix.
Long term, I think it is good to create a new issue to discuss how to structure such tests. It seems it won't be easy, for example only in sanitizeDataForNewSchema
we would have to create 4 crafted test cases that would trigger the retrieveSchema
and then validate if the argument is passed correctly. We don't have such tests for experimental_defaultFormStateBehavior
either as the tests would be really cumbersome. Doing it as a part of this PR would prolong it significantly and leave the library in the broken state.
Also consider adding test for all the modified functions to ensure the param is passed through properly? I know that's a bunch more work, so if you can justify why it is unnecessary, I'll listen.
@heath-freenome I definitely agree that history of this screams that it needs tests.
In the short term, as it acts broken now (the custom merger is called let's say in 50% cases in real world application), I think there are two options: completely revert the feature (which I don't prefer), or merge this fix.
Long term, I think it is good to create a new issue to discuss how to structure such tests. It seems it won't be easy, for example only in
sanitizeDataForNewSchema
we would have to create 4 crafted test cases that would trigger theretrieveSchema
and then validate if the argument is passed correctly. We don't have such tests forexperimental_defaultFormStateBehavior
either as the tests would be really cumbersome. Doing it as a part of this PR would prolong it significantly and leave the library in the broken state.
Fair enough. Can you write an issue to add these tests AND then assign it to yourself? Since you introduced the feature, it makes sense for you to come up with something that covers the test cases. At least for this feature. Maybe we also need a issue for the experimental_defaultFormStateBehavior
as well?
@heath-freenome I did. https://github.com/rjsf-team/react-jsonschema-form/issues/4385
However I cannot assign myself because of the permissions probably.
Do you please plan to release 5.23.0
soon? Thanks!
Reasons for making this change
Unfortunately, in the original PR https://github.com/rjsf-team/react-jsonschema-form/pull/4308, and I am deeply sorry, that I've missed one code branch that the argument must be passed to that led to many other branches to be missed. In this PR the argument is passed in 100% places it should be.
I've had also forgotten to add params to documentation which I've done now.
However, I've tested the code in a real world application and this change really helps the performance. When using our custom fast merge the input event duration went from ~300ms to ~80ms in large form.
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.