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

Using imported object for style attribute is reevaluated if other attribute uses updated signal #1938

Open
aquaductape opened this issue Oct 30, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@aquaductape
Copy link

aquaductape commented Oct 30, 2023

Describe the bug

As the title, when using imported object for element style attribute, it is reevaluated if other attributes that use signal that is updated.

In this example a button element style uses an imported object called style which has background value of red.

export const style = {
  "background": "red",
}

After the button is mounted then it's background is changed to blue via element style property.

import { style } from "./style"

// ...
onMount(() => {
  buttonEl.style.background = 'blue'
})

return (
  <button style={style} ... ref={buttonEl}>
    {count()}
  </button>
);
}

This button's aria-label attribute uses count signal which increments when clicked

const [count, setCount] = createSignal(1);
const increment = () => setCount(count() + 1);
let buttonEl;

onMount(() => {
  buttonEl.style.background = 'blue'
})

return (
  <button type="button" onClick={increment} aria-label={count().toString()} style={style} ref={buttonEl}>
    {count()}
  </button>

Your Example Website or App

https://playground.solidjs.com/anonymous/804fbf3b-2166-430f-ab99-c2b78770a1a6

Steps to Reproduce the Bug or Issue

  1. Click counter button

Expected behavior

For button's background color to remain blue, but after clicking button it resets red which is the background property from object style. If button style attribute uses style2 object, which is not imported then the button remains blue after updating count signal https://playground.solidjs.com/anonymous/efc28da8-a36c-42e5-b109-5d7b8a8c145e.

Screenshots or Videos

No response

Platform

  • OS: macOS
  • Browser: Chrome
  • Version: 118.0.5993.117

Additional context

No response

@ryansolid
Copy link
Member

Thanks for a more realistic example of the effects grouping side effect. This is by design as of right now but this is exactly the type of issue I was looking for.

@aquaductape
Copy link
Author

aquaductape commented Oct 30, 2023

Thanks for a more realistic example of the effects grouping side effect. This is by design as of right now but this is exactly the type of issue I was looking for.

Setting style properties individually, rather than just the object, is the workaround. It doesn't look as clean and requires explanation to new solid users on the grouping side effect, as well as setting the style to the object local in the file works but not an imported one.

import { styleImported } from "./style.ts";

const styleLocal = { /* ... */ }

<button
  aria-label={count().toString()}
  // doesn't revert inside effect
  style={{
    background: styleImported.background,
    "font-size": styleImported["font-size"],
    color: styleImported.color,
    width: styleImported.width,
  }}
  
  // doesn't revert inside effect
  // style={styleLocal}
  
  // reverts
  // style={styleImported}
>
  {count()}
</button>

@ryansolid
Copy link
Member

I remember now what's up. This is a result of a different bug fix from a long time ago. It's because style operates on objects that can have getters so it wasn't sufficient to have it track only if there is function call. And we can't early exit on object reference because it could presumably be mutated. So all we can do is diff. To be fair you'd have this problem if you changed only one style property that wasn't that one. Obviously in this case you are thinking well this is static. But potential to fall back to diffing and last one wins can still happen even without grouped effects. Mutating a DOM reference with the same field you bind to I'm leaning towards just saying you shouldn't have expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants