CSS encapsulation #677
CSS encapsulation #677
Conversation
Just had a quick read, super interesting, but this is just to notify about a couple of nits. |
Co-authored-by: Robert Mosolgo <rmosolgo@github.com>
Thanks @elia!
This seems like a very interesting approach to solving for the use cases described. My comment is more a philosophical one: should this be part of ViewComponent at all? I can think of some reasons why VC could simply provide a few hooks in the base class(es) so that this particular CSS solution would function through a separate gem. That way (a) it's optional, and (b) others could build different solutions in a similar fashion. To be perfectly honest, I never use CSS modules (aka fingerprinting class names) nor HTML-embedded stylesheets, and prefer custom properties, custom elements, shadow DOM, etc. for encapsulation techniques…and so this isn't a solution I personally would use. |
This is great! I'll take some time to try it next week, but here are some preliminary thoughts after reading the PR:
|
|
||
# Generate a short, random-ish token to prevent CSS selector collisions. | ||
def compute_hash | ||
Digest::MD5.hexdigest(@module_name).first(5) # rubocop:disable GitHub/InsecureHashAlgorithm |
Spone
Mar 26, 2021
•
Collaborator
What about using the first 5 characters of the Base64 representation of the hexdigest? This way there's more "specificity" in 5 chars, and even less risk of collision.
- inspired by
css-loader
'slocalIdentName: "[path][name]__[local]--[hash:base64:5]"
(source) - see https://stackoverflow.com/a/9987466/1789900 for an example of implementation
|
||
# Generate a short, random-ish token to prevent CSS selector collisions. | ||
def compute_hash | ||
Digest::MD5.hexdigest(@module_name).first(5) # rubocop:disable GitHub/InsecureHashAlgorithm |
keithamus
Mar 26, 2021
Member
MD5
is pretty good at generating (mostly) collision free hashes, but a couple of points:
- It's counted as encryption and federal compliance regulations have specific rubrics around encryption standards. Using this for "enterprise" software involves some bureaucratic complexity. It might be easier to migrate from this just to avoid having the conversation.
- JS does not have MD5 in its standard library. It's unlikely to get it as it's now an obsolete standard. It's also a non-trivial implementation; it requires quite a lot of code to implement or pull in, so if we need to mirror these on the client side we'd be looking at adding 1-2kb of JS code just to do this.
Perhaps a better option is to use a simpler digest mechanism, such as base64
? is probably more ubiquitous than MD5, and where it isn't it can be implemented in a dozen or so LOC. It does have a much higher potential for collisions; but given it's used as part of a class selector here, the risk of collisions is likely the same as using MD5.
Spone
Mar 26, 2021
•
Collaborator
The problem with simple truncated base64
is that you'll have the same value if components have the same prefix -- it's only encoding the string with a different radix, not hashing it:
Base64.encode64("HelloWorld").first(5)
=> "SGVsb"
Base64.encode64("HelloThere").first(5)
=> "SGVsb"
I'm not sure if there is some other hashing function that does not imply the compliance regulations you mention?
keithamus
Mar 26, 2021
Member
I don't think this is necessarily a problem as the class names would still be different as @module-name
is used both undigested and digested parts of the selector: a full class would be .HelloThere_SGVsb_foo
or .HelloWorld_SGVsb_foo
. The intent of the compute_hash
function seems to be to provide enough entropy in a class name so as not to collide with human authored classes; .HelloThere_foo
is more likely to already exist in existing CSS.
Co-authored-by: Simon Taranto <srt32@github.com>
So happy to finally see this released! How can we support porting existing SCSS code and running tools like postCSS and CSSLint? Using I'm under the impression that for some application would be as fine to just ship the collected CSS of all existing components. That has the advantage of leveraging the browser cache, or not even parsing the bundle more than once by using Turbo. This approach retains most of the properties/advantages as the current solution but simplifies a lot the delivery. I'd like to see a configurable CSS scoping prefix, ideally I should be able to replicate popular solutions like turning That's all I've got for now, hope this feedback will help in shaping a solution that can be effective for apps of all sizes! PS. My experience building two e-commerce stores with CSS and components has been great, we implemented a poor man's version of styled components by just using unique CSS class names based on the Ruby class name. In one case it was built from scratch and we only included a reset and a micro CSS framework. In the other there's plenty of legacy CSS, so far the extraction has been quite painless. The only headaches we had were around components accepting blocks which potentially have conflicting CSS class names and clashes with names coming from utility classes. |
Overall this looks good to me! |
|
||
We considered allowing for evaluation of Ruby inside CSS files (at compile, runtime, or both) but instead decided to lean on [CSS custom properties](https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties) for injecting dynamic values. | ||
|
||
### Bundle splitting |
BlakeWilliams
Mar 29, 2021
Member
It's a bit more broad since it could also support JavaScript, but is it also worth considering svelte style components? https://svelte.dev/
e.g. Instead of having a sidecar file, you would define the style tag directly in the component and we would automatically extract it.
<%# the component erb template %>
<style>
.my_class {
color: red;
}
</style>
<h1 class="my_class">Hello world!</h1>
Which could result in something like (omitting the style or link tag):
<h1 class="abc123_my_class">Hello world!</h1>
I don't think the two approaches are mutually exclusive, but thought it might be worth bringing up.
koddsson
Mar 30, 2021
Member
I think we should eventually support "sidecar" components as well as "svelte" encapsulating components. Having both options would allow you to write little encapsulating components and large sidecar ones.
For simplicity's sake, we should only support sidecars in the beginning.
|
||
### No Webpacker | ||
|
||
The proposed design avoids Webpacker and other build tools. Instead, it uses a Ruby implementation of CSS Modules built by @rmosolgo, enabling greater portability and a convention-over-configuration experience that Rails developers expect. |
BlakeWilliams
Mar 29, 2021
Member
I'm curious how this will scale in Ruby compared to using an external process like webpack or some other build tool. Not a blocker since we can always find ways to optimize, but thought it was worth bringing up.
I also think we may have to revisit this if we look at JS encapsulation. Kind of related to No SCSS
, I think that would be a great time to investigate something like postCSS too.
@@ -10,7 +10,7 @@ def compiled? | |||
CompileCache.compiled?(component_class) | |||
end | |||
|
|||
def compile(raise_errors: false) | |||
def ensure_compiled(raise_errors: false) |
BlakeWilliams
Mar 29, 2021
Member
Should we have a separate compile method that always compiles?
Maybe compile!
?
def test_it_doesnt_rewrite_bare_element_selectors | ||
before_css = "span {\n background: rbga(255, 255, 255, 0.8);\n}" | ||
after_css = "span {\n background: rbga(255, 255, 255, 0.8); }\n" | ||
assert_rewrite("item", before_css, after_css) |
BlakeWilliams
Mar 29, 2021
Member
This allows for global styles, can we make this raise to disallow escaping the component scope?
def commenter_name | ||
commenter.name | ||
def call | ||
content_tag(:div, "Hello, World!", class: styles['foo']) |
BlakeWilliams
Mar 29, 2021
Member
content_tag(:div, "Hello, World!", class: styles['foo']) | |
content_tag(:div, "Hello, World!", class: styles.foo) |
Thoughts on using methods instead of hashes? I think API wise, methods are a little easier to use plus it gives us the ability to later "enhance" the style methods with additional functionality. e.g. Finding unused CSS in the test environment
|
||
### No SCSS | ||
|
||
We're not very sure about this one, but we're playing with the idea of just using plain CSS, at least to start. |
koddsson
Mar 30, 2021
Member
Am I understanding the code correctly that we are using a SASS compiler to generate and traverse the CSS AST?
Spone
Mar 30, 2021
Collaborator
Yes it seems. I was also a bit surprised when I saw the added sass
dependency.
|
||
We considered allowing for evaluation of Ruby inside CSS files (at compile, runtime, or both) but instead decided to lean on [CSS custom properties](https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties) for injecting dynamic values. | ||
|
||
### Bundle splitting |
koddsson
Mar 30, 2021
Member
I think we should eventually support "sidecar" components as well as "svelte" encapsulating components. Having both options would allow you to write little encapsulating components and large sidecar ones.
For simplicity's sake, we should only support sidecars in the beginning.
|
||
## Consequences | ||
|
||
o, avoiding several common errors and resulting in more resilient user interfaces. |
Co-authored-by: Kristján Oddsson <koddsson@gmail.com>
@koddsson @BlakeWilliams @jaredcwhite @Spone @elia @keithamus @srt32 thanks for taking the time to review! Should this be a part of ViewComponent at all?
I think this is a fair point, Jared. As proposed here, one would need to include the Class naming conventionsThere is a lot of subtle complexity here, and the proposed approach is ultimately quite naive. For example, it does not consider the case where a ViewComponent's CSS is updated, but a stale copy of the ViewComponent exists in a view cache. For that reason and others, I expect we'll eventually want to use a hash of the ViewComponent and its sidecar files to generate the fingerprint. API for Slim templates@Spone, thanks for bringing this up! We use ERB exclusively at GitHub, so it's easy to miss these kinds of things. Given the desire to have fingerprinted CSS selectors, could you suggest how we might implement them in a more Slim-friendly fashion? Third-party CSS@Spone, I don't think this proposed architecture would work in that case. I'd expect a consumer of a library like Flickity to write ViewComponents that wrap its behavior. But perhaps you're suggesting that there might be a way to fingerprint third-party CSS? Caching
How can we support porting existing SCSS code and running tools like postCSS and CSSLint?I think that Ship the collected CSS of all existing components@elia, you're right. For a lot of applications, this is probably the correct approach. |
Hey everyone!
I would say "I don't think so". Here is how autoprefixer-rails works: it ships with the prebuilt JS script containing both PostCSS core and Autoprefixer (PostCSS plugin). During assets compilation, it just calls JS runtime (via ExecJS) to run this script against a CSS text. Packing every single PostCSS plugin this way doesn't sound like a good plan. Now, let me share my take on other questions. First of all, I love the idea of using CSS Modules as well as inlining
I'm a bit worried about additional runtime dependencies (currently, However, abstractizing
Maybe, there is already the answer to the question in the thread but: why can't we rely on the component name only? E.g.,
I've been thinking of using shortcuts for that, smth like: p.regular-class$container // expands into <p class="regular-class #{styles['container']}"></p> Not sure that this is possible but I would like to experiment with this. And the last but not least.
I'm a big fan of the classic Rails way and HTML-first approach. But ignoring the fact that frontend ecosystem has greatly progressed since the Golden Age of Asset Pipeline (like, about 10 years ago). PostCSS and others changed the way we write CSS. Tailwind is getting more and more popular in the Rails community (and not everyone hates I think the statement "enabling greater portability" is over-estimated. A lot of projects won't be able to use Stylable due to the dependency on PostCSS, Autoprefixer or whatever. That brings us back to the question whether this particular implementation should be the default one or not. Maybe, we can provide a common API to work with both Ruby CSS Modules and postcss-modules? That would allow us to satisfy everyone. |
Quick try: Spone/slim#1 (requires some minor changes to the ~text Lorem ipsum
/ <div class="Styled_6c266_text">Lorem ipsum</div>
p~text.is-big LOREM IPSUM
/ <p class="Styled_6c266_text is-big">LOREM IPSUM</p> |
To follow up on Third-party CSS, here is an example to make it a bit more concrete. Let's consider a Currently, I see 4 possible approaches:
//= require flickity
// app/components/gallery_component/gallery_component.js
import Flickity from "flickity";
import "flickity/dist/flickity.css";
In approaches To sum up, I'm not sure we need to tackle third-party CSS encapsulation (ie. alter the selectors), at least not in the initial implementation. (@joelhawksley you mentioned fingerprinting, in the case of NPM packages, maybe the |
if !rendered_components.include?(rendered_components_identifier) | ||
request.params["___rendered_view_components"] << rendered_components_identifier | ||
|
||
content_tag(:style, self.class._css.html_safe) |
halo
May 11, 2021
Contributor
I saw that github.com uses the permissive content security policy style-src 'unsafe-inline' github.githubassets.com
.
I suppose unsafe-inline
is then required for anyone using these styleable view components because of the rendering of <style>...</style>
directly into HTML.
Please read this pull request as a proposal that can and should change.
This pull request introduces a prototype implementation of CSS encapsulation. We've been running this code in production for about a month without issue.
Wading into this area is not something we take lightly, and we recognize the significant amount of prior art and context to take into account.
I've included extensive documentation in the form of an Architectural Decision Record. I'd love to see us use that document as the grounds of debate for this proposal.
I'd love to hear your thoughts at all abstraction levels, from the technical details to the broad concepts.