guillotinaweb / ngx-schema-form

HTML form generation based on JSON Schema
MIT License
483 stars 173 forks source link

Prevent multiple calls of _bindVisibility for the same elements when traversing the tree structure #449

Closed KBroichhausen closed 1 year ago

KBroichhausen commented 1 year ago

Let's take the following schema:

{
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "object",
    "title": "Root",
    "properties": {
        "firstLevel": {
            "type": "object",
            "title": "First Level",
            "properties": {
                "oneChildInteger": {
                    "type": "integer",
                    "default": 80,
                    "title": "Integer"
                }
            }
        }
    }
}

https://github.com/guillotinaweb/ngx-schema-form/blob/84d3f8b2573bc5fc3be2831c92b1a0bca97e1e2b/projects/schema-form/src/lib/model/formproperty.ts#L554-L572

1 Issue / Commit For the root property of the schema / _bindVisibilityRecursive is called, which calls forEachChildRecursive with a function fn, where fn is simply calling _bindVisibility on each property. For each child function fn is called and after that the check if (child instanceof PropertyGroup) { calls recursively the function fn via forEachChildRecursive. Since PropertyGroups class already contains _bindVisibilityRecursive for all childs _bindVisibility is called twice for each child.

Maybe a little bit easier explained. If fn already calls itself forEachChildRecursive, you call it twice in the case of the child being an instance of PropertyGroup.

It gets even worse when you think about a secondLevel. The root would call twice the first level, were each one will call twice the second level. This will end up in a total of 4 calls. Well, you know the formula ;)

2 Issue And sadly another issue: The FormPropertyFactory calls the _bindVisibility of each property it is creating in createProperty if it is an instance of PropertyGroup. In the example with a secondLevel that means that the _bindVisibility of oneChildInteger is called: 1x for secondLevel 2x for firstLevel (calls two times secondLevel) 4x for root (calls two times firstLevel, which calls four times secondLevel)

A total of 7 calls for just one binding.

Now, imagine I have a huge json with 122 visibleIf and parts with a nested structure of depth 5. The calls to initially bind everything takes up to 12 seconds on my machine.

A fix for the first issue is quite easy: Just remove the duplicate call and it drastically reduces the time.

The second issue is also improved by that: Now, you just get: 1x for secondLevel 1x for firstLevel (cause it calls secondLevel) 1x for root (cause it calls firstLevel)

But still it get's called n times, where n is the depth of the element within the tree. Fixing this is not that easy in my opinion. Technically, you would need some sort of state for createProperty. The call to _bindVisibility only needs to be done for the "root cause" of the creation. E.g. I initialise everything, so the root cause is the creation of /. Just call _bindVisibility of / and due to recursive child calls everything is bound. E.g. I dynamically add a sibling anotherFirstLevel at the same depth of firstLevel. Just call _bindVisibility of anotherFirstLevel and due to recursive child calls everything is bound and nothing changes for firstLevel or /.

I'm not quite sure about the second example. If a child of firstLevelcontains a visibleIf condition referencing something in anotherFirstLevel, does firstLevel needs to be bind again or does it "just finds it" because now the canonical path is available?