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

set_style optimization silently breaks on invalid style property value #5204

Closed
trbrc opened this issue Jul 25, 2020 · 5 comments · Fixed by #5923
Closed

set_style optimization silently breaks on invalid style property value #5204

trbrc opened this issue Jul 25, 2020 · 5 comments · Fixed by #5923
Labels
bug compiler Changes relating to the compiler documentation temp-stale

Comments

@trbrc
Copy link
Contributor

trbrc commented Jul 25, 2020

Describe the bug
Svelte tries to optimize style attributes to use node.style.setProperty(...) instead of node.setAttribute("style", ...) in certain circumstances. The syntax makes it look like string interpolation, so you would expect it to always have the same result as interpolating the string and setting it as a regular attribute. But the optimization relies on some undocumented assumptions about how you would use these dynamic values, and breaks in some cases.

To Reproduce
Here is an example of some cases that breaks when using Svelte's curly brace syntax, but works with the corresponding JS string interpolation:
https://svelte.dev/repl/9d64d75662a04f049b7724cfe38bca73?version=3.24.0

Expected behavior
Given the lack of documentation or warnings, I simply expect the styles not to break, regardless of how I do the string interpolation.

As an acceptable alternative, the behavior could be documented, and compiling in dev mode could warn when the optimization fails.

Severity
It's probably a niche case, and there are a variety of workarounds. Still, the current behavior could be very confusing and should probably documented.

@trbrc
Copy link
Contributor Author

trbrc commented Jul 25, 2020

Another option would be to make the optimization more explicit by introducing a style: directive, following the pattern of the class: directive:

<h1 style:border={string}>...</h1>

@Conduitry
Copy link
Member

Something like this issue did come up in the discussion on #455 but it was deemed not worth the worry, and this optimization has been in place for nearly three years (via #810), and I think this is the first time I've heard about anyone saying they were trying to do something like this, so it does indeed seem pretty niche, and I think the "don't worry about it" decision has been more or less borne out.

I'm going to label this as a documentation issue. We've been pushing people for a long time to use the style="" syntax, since it's optimized, so we don't want to suddenly stop optimizing it (and make everyone's compiled code slower) and implement a new syntax solely for the purpose of regaining that optimization.

@pngwn
Copy link
Member

pngwn commented Jul 25, 2020

Just to jump on this, do you think it would be possible to apply the same optimisation to:

<div style="{prop}: {value}" />

It is definitely possible at runtime to use (set_style(div, prop, value)) and it feels like this is parsable if we add the same constraints for the name as for the value.

Thoughts?

@pngwn
Copy link
Member

pngwn commented Jul 25, 2020

I raise this due to the inconsistency more than anything, if we double down on this we could optimise further.

Edit: I also appreciate that this is a different issue.

@trbrc
Copy link
Contributor Author

trbrc commented Jul 25, 2020

We've been pushing people for a long time to use the style="" syntax, since it's optimized, so we don't want to suddenly stop optimizing it (and make everyone's compiled code slower) and implement a new syntax solely for the purpose of regaining that optimization.

I briefly looked through the documentation to see where style might have been pushed as optimized, and the main thing I found was some examples of tweened stores used to animate a CSS property.

I can see how it would be unfortunate if the performance there was lost for existing code, but I would still argue that the current API is flawed, and that there would be merit to making style consistent with class. Existing code would continue to work with reduced performance, and would usually be trivial to refactor:

   <div
-   style="transform: translate(0,{-y * layer / (layers.length - 1)}px)"
+   style:transform="translate(0,{-y * layer / (layers.length - 1)}px)"
   >

I'm not going to die on this hill, but another problem I've run into with the current syntax is that styles are always deoptimized if you try to mix blocks of CSS with individual values. This can be very useful to compose dynamic styles, and is a common pattern in CSS-in-JS. As a slightly fictitious example:

<script>
  import {tweened} from 'svelte/motion';
  import {buttonThemeStore} from './theme-stores.js';

  const width = tweened(50);
</script>

<button
  style="
    {$buttonThemeStore};
    width: {$width}px;
  "
>
  ...
</button>

The width is not optimized to use .style.setProperty here, and as far as I can see there's no way to make that happen. With a style directive that wouldn't be a problem:

  <button
    style="
      {$buttonThemeStore};
-     width: {$width}px;
    "
+   style:width="{$width}px"
  >

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler Changes relating to the compiler documentation temp-stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants