svelte-form icon indicating copy to clipboard operation
svelte-form copied to clipboard

Runaway reactivity

Open mustafa0x opened this issue 2 years ago • 4 comments
trafficstars

Wonderful project!

I noticed a strange issue. On each keystroke, helpers.getComponent fires: twice for each input. So if a form has 20 items, it fires 40 times on every key stroke.

https://github.com/pyoner/svelte-form/blob/master/packages/lib/src/helpers.ts#L140

This has performance impacts and could cause unexpected bugs. This seems to be occurring due to the nested bind:values. I tried to resolve this issue, but didn't achieve much.

I'd appreciate if this can be looked into!

mustafa0x avatar Dec 13 '22 01:12 mustafa0x

@mustafa0x I'll check Thanks!

pyoner avatar Dec 13 '22 05:12 pyoner

I resolved this by simply pulling the <svelte:component> props out into variables. (I didn't yet test if this has side effects though. It might have if changing the form props. I can try to avoid that situation though.)

Form.svelte

@@ -4,8 +4,8 @@
     {...getPropsFromContainer(components.layout)}>
     <div slot=fields>
       <svelte:component
-        this={getComponent(schema, components.fields[schema.type], schema.type)}
-        props={getProps(schema, components.fields[schema.type], schema.type)}
+        this={cmp}
+        props={props}
         {components}
         {schema}
         bind:value
@@ -69,4 +69,6 @@ $: if (components && validator) {
         ],
     }
 }
+let cmp = getComponent(schema, components.fields[schema.type], schema.type)
+let props = getProps(schema, components.fields[schema.type], schema.type)
 </script>

Wrap.svelte

@@ -1,7 +1,7 @@
 {#if schema && component}
   <svelte:component
-    this={getComponent(schema, component, key)}
-    {...getProps(schema, component, key)}
+    this={cmp}
+    {...props}
     {schema}
     {errors}>
     <slot />
@@ -16,4 +16,6 @@ export let schema
 export let component
 
 const key = 'wrapper'
+let cmp = getComponent(schema, component, key)
+let props = getProps(schema, component, key)
 </script>

Update: yes, this solution does prevent the form updating when props change. However, this solution works (for Form.svelte) and doesn't cause runaway reactivity:

-let cmp = getComponent(schema, components.fields[schema.type], schema.type)
-let props = getProps(schema, components.fields[schema.type], schema.type)
+$: cmp = getComponent(schema, components.fields[schema.type], schema.type)
+$: props = getProps(schema, components.fields[schema.type], schema.type)

However, there is a harder issue to work around. bind:value={value[key]} in ObjectField.svelte causes a lot of reactivity for any non-trivial schemas. Removing it obviously completely breaks things.

mustafa0x avatar Dec 24 '22 20:12 mustafa0x

Ok, for ObjectField.svelte this works.

@@ -2,8 +2,8 @@
   <Wrap {schema} {errors} component={components.wrapper}>
     {#each Object.entries(schema.properties) as [key, propSchema] (key)}
       <svelte:component
-        this={getComponent(propSchema, components.fields[propSchema.type], 'field')}
-        props={getProps(propSchema, components.fields[propSchema.type], 'field')}
+        this={map[key][0]}
+        props={map[key][1]}
         {components}
         schema={propSchema}
         bind:value={value[key]}
@@ -26,4 +26,11 @@ export let components = p.components
 $: if (schema && value == null) {
     value = defaultValue(schema, value)
 }
+
+const map = Object.fromEntries(
+    Object.entries(schema.properties).map(([key, propSchema]) => [key, [
+        getComponent(propSchema, components.fields[propSchema.type], 'field'),
+        getProps(propSchema, components.fields[propSchema.type], 'field'),
+    ]])
+)
 </script>

mustafa0x avatar Dec 24 '22 23:12 mustafa0x

Update: yes, this solution does prevent the form updating when props change. However, this solution works (for Form.svelte) and doesn't cause runaway reactivity:

-let cmp = getComponent(schema, components.fields[schema.type], schema.type)
-let props = getProps(schema, components.fields[schema.type], schema.type)
+$: cmp = getComponent(schema, components.fields[schema.type], schema.type)
+$: props = getProps(schema, components.fields[schema.type], schema.type)

@mustafa0x thank you for the solution

Yes, we should keep reactivity:

$: cmp = getComponent(schema, components.fields[schema.type], schema.type)
$: props = getProps(schema, components.fields[schema.type], schema.type)

pyoner avatar Dec 25 '22 09:12 pyoner