search

guillotinaweb / ngx-schema-form

HTML form generation based on JSON Schema
MIT License
485 stars 174 forks source link

Regression fix and further performance boost #450

Closed KBroichhausen closed 1 year ago

KBroichhausen commented 1 year ago

First patch fixes an issue with wildcards. I added chainable allOf and oneOf in a previous patch but misunderstood the findProperties and immediately returned after first one was processed. So, the second, third etc. elements were not used.

Second patch is for performance. I think, I eliminated all unneccessary multiple calls to bindVisiblity method. When root element is created, it already creates all child elements / properties but now no call to bindVisiblity is made. Just after the root element's creation is done, bindVisibility is called, which also processes all childs. So far this works fine for me because the ExtendedProxyHandler already takes care of rebinding on changes (like adding an element to an array). I hope there are no other use cases which will fail with this patch.

KBroichhausen commented 1 year ago

Readded the reset of propertyGroups to pass the tests :)

KBroichhausen commented 1 year ago

Found another bug with this change...on hold

KBroichhausen commented 1 year ago

@ebrehault Just realised that this has nothing to do with my changes because it is also in 2.8.1.

Would it be accepted to modify the following part such that it removes all keys from this.model and then Object.assign(this.model, value)? This makes the model equal to the visible elements values. If something is removed from value because it is not visible anymore, it is not removed because Object.assign does not do that. Using = is also bad because this destroy the references to the this.model object?!? But is there a more effective way to remove all properties from one object and then copy all properties from another object into it instead of iterating through all keys, remove them and use Object.assign? Basically, it's a copy but I need the reference to stay the same. That would be a simple memcpy in C.

My application assumes that all elements in the model are visible. At initial state some elements are not visible but after they are added they never get removed. Like I said, this is also the cause in 2.8.1 but I never realised it. With the patch in this pull request all elements are temporarily visible (because I split creation of properties and visibility binding in two "processes") and therefore the problem is very present now. https://github.com/guillotinaweb/ngx-schema-form/blob/821c10723032b0444757332af2c3515e733cb172/projects/schema-form/src/lib/form.component.ts#L200-L206