Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ungroup dynamics for universal renderer #157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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