Styles are not unset when the style object changes

[ricochet1k]

Sandbox: https://codesandbox.io/s/style-doesnt-unset-0zb01?file=/index.js

    <div style={state.foo ? { background: "green" } : { color: "green" }}>
      hi
    </div>

This should toggle between green background black text and white background green text, but instead after a second it just ends up as green for both, because style is not unset when the keys disappear.

[ryansolid]

Hmm.. That's interesting. I've never used styles this way but that is probably coming from a Knockout or binding library syntax perspective. How important is I wonder. Mostly it's just much more complicated to do styles this way (way more code and it would be slower). This makes a big deal when doing animations. But seeing it I can see it's completely legal though so I can see how it could cause confusion.

So obviously I'm like just write:

<div style={{ background: state.foo ? "green" : "", color: !state.foo ? "green" : "" }}>
   hi
</div>

But I appreciate that is likely an unreasonable ask. Maybe I can't get away with just documenting it.

There is a way to make it work and be performant but it would be taking a page from inferno's book which means using style.setProperty although that would lead to background-color rather than backgroundColor. See: https://github.com/infernojs/inferno/blob/master/documentation/v6-migration.md#style-property-names-changed

EDIT: For reference... Currently DOM Expressions uses:

createEffect(() => Object.assign(el.styles, styleProp))

To do removals we need to store previous value and iterate over the new object keys and then iterate over the old object keys which are not present in the new object keys. At that point you can either set the property on the styles object or use setProperty. Iterating and setting the property is slower whereas setProperty method is actually even with iteration faster even than Object.assign but you need to use the string versions of css properties.

Here is the inferno implementation: https://github.com/infernojs/inferno/blob/master/packages/inferno/src/DOM/props.ts#L34

[ryansolid]

I actually want to open this up and get some feedback here. I'm not opposed to Inferno's approach, but I think it might hangup some people from a React perspective. Also maybe from a TS perspective. There is this weird inconsistency between property/attribute APIs so straddling the line always feels a bit inconsistent even with clear heuristics.

[trusktr]

What if we parallel to DOM standards, and otherwise make new APIs with improved features for clarity. For example, the style attribute is normally a string. So:

style={state.foo ? "background: green" : "color: green"}

And for optimization,

fastStyle={state.foo ? { background: "green" } : { color: "green" }}

As for the actual fix, the simplest code might currently be:

createEffect(() => {
  if (el.attributeStyleMap) el.attributeStyleMap.clear()
  else el.style = ""
  Object.assign(el.style, styleProp)
})

As for the Object.assign(el.style, styleProp) part, maybe it is faster to use the CSS Typed Model there too, as opposed to the old CSSStyleDeclaration model. According to MDN some browsers don't support it yet, hence the conditional check.

[trusktr]

I'm not sure of the performance implications. Maybe tracking a cache object and strategically applying the properties may be faster, but that code is definitely simpler.

I can make a PR with the simple version if you approve @ryansolid

[trusktr]

Oh wait, so, styleProp in the above example is a single reference that doesn't change, isn't it?

Oh wait, nvm, the OP described that the new object is used when state changes, so Solid updates the reference. Then that idea should work.

[ryansolid]

yeah unclear on performance. My hunch is always resetting is expensive. What i do know is:

//setProperty
el.styles.setProperty({ "background-color": "red" })
//outperforms
el.styles.backgroundColor = "red";

In fact Inferno's approach is faster than Solid's here. But no camelCase. And need to include units. I don't care about size that much but I do performance,

[ryansolid]

Maybe a better question. Is there any objections/concerns about changing the implementation to be the same as Inferno? Am I missing some scenarios?

[ryansolid]

Ok here's the solution I propose.

1. Switch to setProperty in general as it is more performant, supports more css expressions, and closer aligns with SSR.
2. My proposed form is statically analysable. It is more efficiently transformed. So we compile it straight into unwound statements with no extra runtime code Ie.

   
   <div style={{ background: state.foo ? "green" : "", color: !state.foo ? "green" : "" }}>
   hi
   </div>

// becomes const el = template.cloneNode() createEffect(() => { el.style.setProperty("background", state.foo ? "green" : ""); el.style.setProperty("color": !state.foo ? "green" : "" })

3. Everything else imports a helper which iterates over Object keys of current and past values and set the value as intended.

In this way all cases are consistent behavior and the preferred approach is more optimal and doesnt take a size or performance hit.

[ryansolid]

I'm happy to report this is fixed in v0.18.0.