experimental_customMergeAllOf

[MarekBodingerBA]

Reasons for making this change

NOTE: This is just a draft PR; details are subject to discussion.

In many cases, json-schema-merge-allof is a performance bottleneck for this library:

• There are a few related issues: Use of allOf can trigger laggy interface and "too much recursion" errors #2811. "allOf" Support for the custom JSON schema keywords merging  #3258 (this change allows to use custom keywords for the original library).
• The library is unmaintained, with the last release on 22/01/2021 (4 years ago).
• The library deep clones every schema, and this cloning is called recursively for each call. In large production schemas, this is detrimental to performance.
• There are "new players on the block" such as allof-merge that promise much better performance.
• For our use case, as we only use limited features of JSON Schema, we can use a very fast custom implementation of merging. This largely solves the library's performance issues for us; getDefaultFormState (not just the merging itself) performs 7x faster in our implementation:

getDefaultFormState_original x 141 ops/sec ±2.65% (80 runs sampled)
getDefaultFormState_fastMergeAllOf x 987 ops/sec ±2.72% (77 runs sampled)

I have a couple of questions:

• Is this something that would be desirable to implement for others?
• I would like feedback on the naming and types of experimental_customMergeAllOf. Maybe it's better to provide the function wrapped in an object rather than the function itself.
• schemaParser (which is only used in compileSchemaValidatorsCode) relies internally on retrieveSchemaInternal. I chose not to add this option here; is it necessary to include it?

The functional change is located here, other changes are just adding the argument where needed.

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

[MarekBodingerBA]

When I think about it now, I would probably implement it as an object (that would be easier for future compatibility). I imagine something like this:

export type Experimental_MergeAllOfOptions<S extends StrictRJSFSchema = RJSFSchema> =
  | {
      merger: "default";
      overideOptions: Options; // json-schema-merge-allof options
    }
  | {
      merger: "custom";
      mergeFn: (schema: S) => S;
    }
  | {
      merger: "xyz"; // other library if added in the future
    };

WDYT?

[heath-freenome]

@MarekBodinger I totally understand why you want to add this. We feel like exposing the Options for json-schema-merge-allof is a bad idea. Honestly, if allof-merge has better performance and doesn't break our exists tests, then we welcome a PR to swap it in. That said, making it an object just to provide a function seems like overkill. So making it be a merge function like you started is fine. Not sure whether we can/want to expose this in the playground.

[nickgros]

Also linking this bug: #2927 which may be fixed by an alternative/improved merge implementation

[nickgros]

I tried to just plug in allof-merge and the only meaningful test failure was this one in retrieveSchemaTest. It looks like allof-merge has a lot of customization options, so it may be worth spending a bit of time to see if the behavior can be tweaked to get it to work properly, and we could make it the default implementation (if you're interested in looking into it @MarekBodingerBA)

[MarekBodingerBA]

I don't think it is a good to replace json-schema-merge-allof with allof-merge without thorough testing. As a user of this library, I would be concerned about such a change.

The approach already in progress, which introduces an experimental_customMergeAllOf option, is more appropriate. This would allow interested users who understand the implications to provide their own custom implementation, where they could easily incorporate allof-merge if desired.

I've completed the work on this and appreciate the feedback received.

[nickgros]

LGTM!

[nickgros]

@MarekBodingerBA can you resolve the conflicts? Likely because we merged #4312

[MarekBodingerBA]

@nickgros Unfortunately, I don't have access to a computer for next three weeks. Is it something that you are able to do, or should we wait until I come back?