feat: improve deepEquals performance

[igorbrasileiro]

Reasons for making this change

This PR improves the RJSF performance when comparing objects deeply.
fixes #4291

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]

Just some documentation and then I'll approve

[igorbrasileiro]

Added the JSDoc for new functions.

[heath-freenome]

Can you also fix the merge conflict in the CHANGELOG.md? Otherwise looks good! How is the performance of fast-equals vs fast-deep-equals?

[igorbrasileiro]

I pointed in the issue the performance of the customDeepEqual. But the results

The deepEqualsFunction

const customDeepEqual = createCustomEqual({
  createInternalComparator: (
    comparator: (a: any, b: any, state: State<any>) => boolean,
  ) => {
    return (
      a: any,
      b: any,
      _idxA: any,
      _idxB: any,
      _parentA: any,
      _parentB: any,
      state: State<any>,
    ) => {
      if (typeof a === "function" && typeof b === "function") {
        // Assume all functions are equivalent
        // see https://github.com/rjsf-team/react-jsonschema-form/issues/255
        return true;
      }

      return comparator(a, b, state);
    };
  },
});

[benjdlambert]

We seem to be getting test failures for this upgrade with maximum call stack exceeded backstage/backstage#26639 don't have time to put together a reproduction repo just now, but will try and get to it this week. Just a heads up for now.

[igorbrasileiro]

@benjdlambert thank you for let me know about it. I will try to reproduce this error and fix it.

Does your props have circularity?

[igorbrasileiro]

I discovered how to reproduce. I reverted the deepEquals fn and the problem happened, so the issue isn't the way the fast-equals compare, it was the refactor.

Edit: I was testing on a schema with issues in one of the anyOfs, I added a null option to the anyOf and the loop stopped (explanation here #4262).

[heath-freenome]

@igorbrasileiro Is it possible to programmatically fix it such that the null option is unnecessary?

[igorbrasileiro]

@heath-freenome I don't have enough context about the JSON schema spec and where to start investigating this issue, but I could implement a fix. I think my issue is different from that of @benjdlambert .

Regarding the issue related to this PR, I haven't found a way to reproduce it. Do you know or have a hint?

[heath-freenome]

@igorbrasileiro I know that @nickgros is working on an issue related to this where certain schemas cause an infinite recursion. At the same time, I wonder if your deep comparison changes are affecting a situation where isEqual() returns a value that differs from deepEquals() thereby causing infinite recursion where it was caught before. For your situation at least, if you could debug the before/after results to see how isEqual() vs deepEquals() causes the algorithms around infinite recursion to change, and then either fixing it (if you clearly introduced a bug) or reaching back out if it doesn't make sense to you...

[igorbrasileiro]

Sure, that sounds good to me. What do you think about reverting this PR, then I try to find the issue and reimplement it with the fix?

Edit: Would be good to know how to reproduce the issue.

[heath-freenome]

Sure, that sounds good to me. What do you think about reverting this PR, then I try to find the issue and reimplement it with the fix?

Edit: Would be good to know how to reproduce the issue.

@igorbrasileiro That makes sense... Can you push the PR and then also, work on fixing the problem you experienced. I agree understanding the problem would be super helpful. It's clear that you already have one use case to start with.

@benjdlambert Can you provide us with your schema as well to help us debug.

@nickgros If you get a chance to figure out the infinite recursion issue you are working on, feel free to chime in.

[benjdlambert]

The schemas in the tests were pretty trivial to be honest, I tried to replicate this in a small codesandbox, and think it might have been to do with the fact that the formData might have had some cyclical references in there.

There's an example here: https://codesandbox.io/p/sandbox/n6txcy so if you try and edit the form data it would break.

Granted this example is not really what you would actually do, but it was something that i could replicate the example with. I was trying to find a better way to reproduce this, as it looks like on first render it might be fine, but when it's re-rendered for some reason it causes issues with the lookup, which kind of aligns with what we were seeing in our test suite that all the things looked pretty sane, but when updating the form it hit the maximum call stack exceeded.

Sorry I can't be of much more help right now :( If I can get some more free time I can try and get a better repro example

[igorbrasileiro]

@benjdlambert thank you for the codesandbox!

Changing the onChange function from your example to follow the docs, the error stops.

        onChange={(e) => {
-         setFormState(e);
+         setFormState(e.formData);
        }}

@heath-freenome can you give your considerations here? Adding the whole object received from onChange fn causes cyclic references, as far as I know this is not expected.

[igorbrasileiro]

@heath-freenome as it seems to be a wrong usage of the form, can I revert the revert #4300 ? I'm considering that is not allowed pass props with cyclic reference. If is allowed I can setup the customcomparator from fast-equals to check if cyclic reference.

[benjdlambert]

@igorbrasileiro I'm not convinced that crashing because of circular references in any of the props is great to be honest. I wonder if there's a way to drop the circular references ahead of the diffing or something.

I mentioned in my reproduction Granted this example is not really what you would actually do. We use just e.formData but this was still happening to us in our test cases, was just a quick way I could put together the error.

Let me see if I can get some time today to dig into what is actually happening in the broken version, as when I was debugging our tests the props that we were using looked pretty simple and we're not using cyclical references to what I could see.

[igorbrasileiro]

We use just e.formData but this was still happening to us in our test cases, was just a quick way I could put together the error.

@benjdlambert can you share the schema and formData from your test? I only get maximum call stack size exceeded with a schema with loop, but it also crashed using the form in the previous version.

Environment to reproduce, using deno, https://github.com/igorbrasileiro/rjsf-form-issue

formData: {}, schema:

{
  "definitions": {
    "first": {
      "type": "object",
      "properties": {
        "name": { "type": "string" },
        "children": { "$ref": "#/definitions/second" }
      }
    },
    "second": {
      "type": "object",
      "properties": {
        "name2": { "type": "string" },
        "children": { "$ref": "#/definitions/first" }
      }
    }
  },
  "$ref": "#/definitions/first"
}