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

String styles: please make it easy #1707

Closed
edemaine opened this issue Jun 12, 2019 · 17 comments · Fixed by #1744
Closed

String styles: please make it easy #1707

edemaine opened this issue Jun 12, 2019 · 17 comments · Fixed by #1744

Comments

@edemaine
Copy link

edemaine commented Jun 12, 2019

10.0.0-beta.2 notes say:

We removed support for string styles in favor of setting style properties only with objects. This aligns our behavior with various other virtual-dom based libraries out there ✔️ Nonetheless we are aware that this might be a controversial change. If you have a lot of string styles or want to continue using them, you can use preact-string-styles to patch this behavior back into Preact X. If you don't agree this change, please let us know!

Let it be known: I'd much prefer to allow string styles. Indeed, the lighter-touch approach that keeps Preact JSX markup closer to actual HTML was one of my motivations for choosing it. I'd also be OK with an NPM package that delivers the functionality of preact-string-styles, whereas currently it's a gist.

On the plus side, I'd also like to report that string styles still seem to be working fine for me in this beta, at least when rendering to strings via preact-render-to-string. (In my application, that's all I need -- perhaps the lack of support is in render()?)

In case you're curious about my application: I'm using JSX as a way to write beautiful code that generates SVG. (Here's a simple example.) Thus, I'd like the markup part to look as much like actual SVG as possible. But I realize this isn't the standard application of Preact, so you might want to relegate this feature to a package.

@JoviDeCroock
Copy link
Member

All right, I'll see if I can provide a tested published package as soon as possible!

@JoviDeCroock
Copy link
Member

I'll publish it to npm and probably move it to the preactjs team when I've got the tests set up: https://github.com/JoviDeCroock/preact-string-styles

@pmkroeker
Copy link
Contributor

pmkroeker commented Jun 17, 2019

@JoviDeCroock to make sure this works properly with typescript you should overwrite the typings for HTMLAttributes.style.

// types.d.ts
declare namespace preact.JSX {
  interface HTMLAttributes {
    style?: {[key: string]: string | number} | string;
  }
}

Then TS users can use either strings or the default object notation.

@JoviDeCroock
Copy link
Member

Good point @pmkroeker

@marvinhagemeister
Copy link
Member

I'm wondering if we should bring them back mainly for the reason that we advertise as being "closer to the metal" and string styles fit into that description.

@jeremy-coleman
Copy link
Contributor

jeremy-coleman commented Jun 20, 2019

Style properties are objects though. Also array ops (strings are arrays of characters after all) have an overhead in chrome bc v8 turns the operations on the string into a trie before doing any work. This is why .join() ops tend to have a constant ~30 ms overhead regardless of complexity when similar ops on an array buffer are ~2ms. This is relevant if you want to prevent over writing existing styles. I love preact because its api matches the dom api , and string styles are not in line with that see:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style

I would think something like styled jsx + babel macro would be the best choice, but still overall a bad one. You can always put inline string styles in a style tag where they belong

@PepsRyuu
Copy link

Considering that Preact supports "class" instead of "className" for the very reason that it's closer to the HTML specification and more intuitive, I see supporting string styles to be a natural extension of that philosophy. It's one of the reasons why I actually prefer Preact over React, because it's not trying to strictly tie itself to the messy DOM API like other VDOM libraries.

For me personally, I'm frequently using string styles when working with Preact. They're lighter on the eyes and easier to write. Instead of creating a new class for a simple once off style, I'll inline the style instead. I don't want to have to start putting in several brackets, quotes and commas for what used to be a simple attribute. Also, similar to the OP, I'm also putting generated SVG code into components, and I am not looking forward to having to modify all of the style attributes and convert them to objects.

@robertknight
Copy link
Member

My understanding is that the main non-functional priorities for Preact are small size and good performance, in that order. Support for string styles was removed in pursuit of the first goal.

It's one of the reasons why I actually prefer Preact over React, because it's not trying to strictly tie itself to the messy DOM API like other VDOM libraries.

I would say the opposite is the case: Preact is "closer to the metal" in the sense that there is minimal abstraction of the DOM. The upside is that documentation for DOM APIs and quirks is more directly applicable to Preact than it might be to other libraries which try to abstract away certain things.

Also, similar to the OP, I'm also putting generated SVG code into components, and I am not looking forward to having to modify all of the style attributes and convert them to objects.

If you have a significant amount of existing code that you need to port from Preact 8 to Preact 10, you can take advantage of its "middleware" support, as @JoviDeCroock illustrated in https://github.com/JoviDeCroock/preact-string-styles/blob/master/src/index.js.

@PepsRyuu
Copy link

Under that argument would you suggest that support for both "class" and "for" attributes should be removed? Would you agree in that case that all other HTML attributes, for example "xlink:href" should be removed for SVGs? You can save quite a few bytes by removing support for those attributes, in pursuit of the first goal.

Even React is reverting on the idea of sticking closely to the DOM API: facebook/react#13525

I would argue that the documentation for both HTML and the DOM API is far more applicable to Preact than other VDOM libraries. By removing support for string styles, you're adding confusion because some HTML attributes like class are supported, but then styles arbitrarily aren't supported? The beauty of Preact is that for newer developers, there's an abundance of legacy documentation for HTML that's perfectly applicable under Preact, whereas for React it will break.

I personally think that it's worthwhile spending a few extra bytes, which the middleware clearly demonstrates is insignificant, to save developers a massive headache.

@robertknight
Copy link
Member

You can save quite a few bytes by removing support for those attributes, in pursuit of the first goal.

There is no explicit logic to support the for attribute, it just comes naturally from the fact that JSX props in Preact set the DOM property with that name if there is one or the attribute otherwise. class could work for the same reason (although it doesn't in practice for performance reasons).

I don't have a particularly strong view on the cost/benefit in this particular case, I'm just noting that it requires a little extra code and the general approach, at least recently, has been to keep special-casing to a minimum.

There is currently no method to force use of an attribute or property. I suppose if there was, that might address this in a generic way.

One additional concern with string styles that the React docs mention, is security. I don't know what all the risks are here and I don't think Preact itself has a clear policy on what it does/does not intend to protect developers from, besides arbitrary HTML injection. For example, there is currently no URL sanitization.

@PepsRyuu
Copy link

I don't think support for string styles is special-casing at all. To me, it's restoring support for a widely supported and well understood HTML attribute, into a format that looks a lot like HTML. This is not at all similar to what React has been doing, adding several layers of abstraction around DOM APIs that weren't issues to begin with, or have stopped being issues years ago.

I think there's a clear distinction between implementing the HTML standard for a HTML-like format, and going beyond the standards and changing well-documented browser behaviours.

The React documentation there is incredibly misleading and inaccurate. String styles do not introduce any security concerns whatsoever. XSS via styles is simply not possible with modern web browsers, as they've patched that security flaw a while back. Also, let's assume that there was an injection vulnerability, that still applies to objects, and ironically that same React documentation demonstrates the attack vector.

@jeremy-coleman
Copy link
Contributor

Another thing to consider is interop. With 100% certainty, At some point in the future, every preact component will need to render using a different rendering implementation or be thrown away. A question to ask is, does this feature increase the liklihood my work today will be useless tomorrow

@robertknight
Copy link
Member

Another thing to consider is interop.

Do you mean interop with other React-like libraries or something else? The fact that React doesn't support string styles was one of the considerations mentioned in the release notes.

@PepsRyuu
Copy link

Interop would completely contradict what Preact has been doing so far. With that logic, not only would you have to remove support for HTML/SVG attributes that React doesn't support, but you would have to implement the entire React synthetic event system, along with the onChange/onInput mess. For me personally, interop is not an issue at all. I would only care about React components working in Preact, but not the other way around.

@marvinhagemeister
Copy link
Member

Ok, so after re-reading the thread and everyone's opinion I've come to the conclusion that we should bring string styles back. My reasoning is that we'd break a lot of existing preact apps and we shouldn't do such change so short before a final X release.

String styles are part of the DOM interface after all and we've profited from them in the past back when we lacked support for CSS variables in preact. On top of that having string styles is a lower barrier to entry for beginners as they can just copy & paste their existing html code into jsx and have it work out of the box.

I've made #1744 accordingly which brings back string styles.

@robertknight
Copy link
Member

robertknight commented Jun 29, 2019

My reasoning is that we'd break a lot of existing preact apps and we shouldn't do such change so short before a final X release.

OK, that sounds reasonable to me. As an aside, perhaps we ought to have some optional flag for preact/debug (options.reactCompatWarnings = true ?) which warns when using Preact-specific features that won't work in React.

@marvinhagemeister
Copy link
Member

We just merged the PR and string styles will be included in the next release again 🎉 Thank you all for sharing your concerns 👍 💯

robertknight added a commit to robertknight/client that referenced this issue Nov 14, 2021
The use of string styles is a Preact-specific feature which React
intentionally doesn't support [1] for security reasons. Although Preact
continues to support string styles following an attempt to remove it [2]
I think we should prefer using only the object setter.

[1] https://reactjs.org/docs/dom-elements.html#style
[2] preactjs/preact#1707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants