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

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

Open
martinarvaiaccedo opened this issue Oct 30, 2022 · 7 comments

Comments

@martinarvaiaccedo
Copy link

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!

@marvinhagemeister marvinhagemeister added the bug Something isn't working label Nov 2, 2022
@developit
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

developit commented Nov 22, 2022

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
Copy link
Author

@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
Copy link
Member

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

@JoviDeCroock JoviDeCroock added discussion and removed bug Something isn't working labels Mar 9, 2023
@akbr
Copy link

akbr commented Nov 25, 2023

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

@XantreDev
Copy link
Contributor

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

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

No branches or pull requests

6 participants