Skip to content

Conversation

@vjoao
Copy link
Contributor

@vjoao vjoao commented Aug 6, 2022

Summary

This PR removes the effect grouping for dynamics in the 'universal' generated code

Description

A potential perf issue when complex objects that have reactive props inside them are passed as props in the template.

<element prop1={[ {x: 30}, { y: count() }]} prop2={{ key: anotherSignal() }} />

The transformer will group those 2 props into a single effect, which is neat, but because the values are complex, they will always fail an equality check:

_$effect(_p$ => {
  const _v$ = [{
    x: 30
  }, {
    y: count()
  }],
  _v$2 = {
    key: anotherSignal()
  };
  _v$ !== _p$._v$ && (_p$._v$ = _$setProp(_el$, "prop1", _v$, _p$._v$));
  _v$2 !== _p$._v$2 && (_p$._v$2 = _$setProp(_el$, "prop2", _v$2, _p$._v$2));
  return _p$;
}, {
  _v$: undefined,
  _v$2: undefined
});

Note that _v$ will be a new array on every effect run. _v$2 will be a new object on every effect run. Both will fail the equality check and reassign on any signal change of that effect, be it count() or another().

Removing grouping means you trade memory for execution speed because now dynamics only execute for the props they depend on. If you want this to be a configuration option let me know.

Testing

Unit tests green

@ryansolid
Copy link
Owner

Sorry for the delay on this. I'm weighing doing it this way versus bigger changes we could make and I'd rather not change and change again. Mostly looking if we can change some more around props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants