solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
31.64k stars 887 forks source link

The update of props is not granular enough. #2142

Closed phy-lei closed 2 months ago

phy-lei commented 2 months ago

Describe the bug

const getId = () => {
  console.log("run");
  return "button-id";
};

<button
    type="button"
    onClick={increment}
    data-count={count()}
    id={getId()}
 >
    {count()}
 </button>

Your Example Website or App

https://playground.solidjs.com/anonymous/c4810293-8e55-49c5-a04b-f56408873017

Steps to Reproduce the Bug or Issue

  1. Click on button
  2. See the console

Expected behavior

  _$effect(_p$ => {
      var _v$ = `color: ${color()}`,
        _v$2 = getMyColor();
      _p$.e = _$style(_el$3, _v$, _p$.e);
      _v$2 !== _p$.t && _$setAttribute(_el$3, "id", _p$.t = _v$2);
      return _p$;
    }, {
    e: undefined,
    t: undefined
  });

It seems that the prop updates are wrapped in an effect, giving an impression of a complete replacement. I'm wondering if this aligns with signal's prop updating strategy, which typically emphasizes fine-grained and efficient changes.

Screenshots or Videos

No response

Platform

Additional context

No response

titoBouzout commented 2 months ago

This is what we call "merged effects", instead of adding 1 effect to each prop, it adds an effect that tracks all the props. It's an optimization for avoiding having to create too many effects. You are supposed to use a memo for prop functions that you wish to not re-run.

phy-lei commented 2 months ago

This is what we call "merged effects", instead of adding 1 effect to each prop, it adds an effect that tracks all the props. It's an optimization for avoiding having to create too many effects. You are supposed to use a memo for prop functions that you wish to not re-run.

Thank you for your response. The term 'merged effects' seems to imply a difference from Qwik, where each prop is individually maintained. This appears odd to me that static props still require memoization by the user. However, I'll adhere to this rule.

ryansolid commented 2 months ago

If you look there is shallow diffing taking place here not to over-update the DOM. But yes we loosened the granularity to improve creation speed. The update speed usually is trivial by comparison. Truly static props are hoisted out of the effect but both data-count and id could update so they are there (they call functions so they could be reactive). The way to think of createMemo is the cost of the execution. If you call a function and it is relatively cheap (most things) then it isn't worth wrapping. If it is wrap. This is pretty independent of how it is used.

Finally Qwik gets there grouping from having a VDOM re-render components on non-granular changes, but having granular changes be our only approach has caused this approach to evolve over the past 6 years. The truth of the matter is that purely granular overhead can be significant. This benchmark shows some of the impact as to get good granular scores it pushes you towards a more granular implementation. Especially note benchmarks around creation/removal: https://krausest.github.io/js-framework-benchmark/current.html

phy-lei commented 2 months ago

If you look there is shallow diffing taking place here not to over-update the DOM. But yes we loosened the granularity to improve creation speed. The update speed usually is trivial by comparison. Truly static props are hoisted out of the effect but both data-count and id could update so they are there (they call functions so they could be reactive). The way to think of createMemo is the cost of the execution. If you call a function and it is relatively cheap (most things) then it isn't worth wrapping. If it is wrap. This is pretty independent of how it is used.

Finally Qwik gets there grouping from having a VDOM re-render components on non-granular changes, but having granular changes be our only approach has caused this approach to evolve over the past 6 years. The truth of the matter is that purely granular overhead can be significant. This benchmark shows some of the impact as to get good granular scores it pushes you towards a more granular implementation. Especially note benchmarks around creation/removal: https://krausest.github.io/js-framework-benchmark/current.html

Thanks!! Creation speed escaped my attention.