experimental_customMergeAllOf v2

[MarekBodingerBA]

Reasons for making this change

Unfortunately, in the original PR #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

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• I'm adding or updating code
  • I've added and/or updated tests. I've run npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update to update snapshots, if needed.
  • I've updated docs if needed
  • I've updated the changelog with a description of the PR
• I'm adding a new feature
  • I've updated the playground with an example use of the feature

[heath-freenome]

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]

Can you update this to be part of the upcoming 5.23.0 release instead?

[MarekBodingerBA]

Done.

[MarekBodingerBA]

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.

[heath-freenome]

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.

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?

[MarekBodingerBA]

@heath-freenome I did. #4385

However I cannot assign myself because of the permissions probably.

Do you please plan to release 5.23.0 soon? Thanks!