preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.72k stars 91 forks source link

signals/preact: createPropUpdater, style property as signal does not update #255

Open martinarvaiaccedo opened 1 year ago

martinarvaiaccedo commented 1 year ago

Description

Hi! I've been testing signals to update various properties and found that style for example will not work if the signal is updated. createPropUpdater will not set style properly.

Is it intended to work like this? Documentation will not mention this but by reading the code it has some relevant implementation in createPropUpdater function. I am wondering if the actual propUpdater should technically represent the same features as preact's setProperty does. I suspect that performance-wise it would be a regression to use setProperty but still, it now has a different behavior which may cause some confusion.

Example

At first render it works fine, and sets the style properly. After updating the signal, createPropUpdater will try to set dom.style = signal.value which will not work.

image

const App = () => {
  const styleSig = useSignal({ transform: `translateX(20px)` });

  return (
    <button style={styleSig}
      onClick={() => {
            styleSig.value = { transform: `translateX(${Math.random() * 10}px)` };
      }}
    >Hello</button>
  );
};

Deps

"@preact/signals": "^1.1.2",
"preact": "^10.11.2"

Thank you for looking into this!

developit commented 1 year ago

@martinarvaiaccedo Signals currently supports string style values, but not object style values. To support objects, we'd need to diff the object properties, which starts to erode the purpose of signals being a direct path to the dom.

martinarvaiaccedo commented 1 year ago

Ok, understood. I feel it is a bit sketchy to see just not to work without any error or indication/documentation. By the way, do style properties need to be diffed? Cant be simply updated? I am not sure about the performance problems it might cause. Anyhow, I agree if you say it should not be supported until it is documented in some way :) Thank you for looking into it!

developit commented 1 year ago

The reason style updates need to be diffed is to handle cases like this:

function Demo() {
  const style = useSignal({
    color: 'red',
    background: 'blue'
  });

  function hover() {
    style.value = { color: 'green' };
    // ^ does not unset background
  }

  function unhover() {
    style.value = {
      color: 'red',
      background: 'blue'
    };
  }

  return (
    <button style={style} onMouseOver={hover} onMouseOut={unhover}>
      Hover Me
    </button>
  );
}
martinarvaiaccedo commented 1 year ago

@developit thank you, makes sense! At the end of the day it is possible to implement it in a custom way using refs if someone needs this feature. I'm happy to have this ticket closed if you feel.

developit commented 1 year ago

I'm good leaving this open - it would be good to sync this up with Preact's normal style prop diffing.

akbr commented 9 months ago

Same issue. Can work around it, but +1 to syncing this up eventually. Seems like the more intuitive behavior.

XantreDev commented 9 months ago

Maybe the preact should expose api for transforming object styles to strings and use it in integration. I can provide PR if this api already exist