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

CSS encapsulation #677

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

CSS encapsulation #677

wants to merge 25 commits into from

Conversation

@joelhawksley
Copy link
Contributor

@joelhawksley joelhawksley commented Mar 25, 2021

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.

@joelhawksley joelhawksley marked this pull request as ready for review Mar 25, 2021
Copy link
Collaborator

@elia elia left a comment

Just had a quick read, super interesting, but this is just to notify about a couple of nits.
One of the two is that the first commit is missing a : after Co-authored-by.

adr/0677-css-encapsulation.md Outdated Show resolved Hide resolved
joelhawksley and others added 19 commits Mar 24, 2021
Co-authored-by: Robert Mosolgo <rmosolgo@github.com>
Thanks @elia!
@joelhawksley joelhawksley force-pushed the css-encapsulation branch from d8996d8 to 6bca5ad Mar 25, 2021
@jaredcwhite
Copy link
Contributor

@jaredcwhite jaredcwhite commented Mar 26, 2021

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.

@Spone
Copy link
Collaborator

@Spone Spone commented Mar 26, 2021

This is great! I'll take some time to try it next week, but here are some preliminary thoughts after reading the PR:

  • Generator support: will the component generator be updated to generate the sidecar CSS file? (with a --css flag or global setting)
  • Naming conventions: I see that you went with a .Css_0343d_foo style selector for the foo class in StylableComponent. Most of the time, my personal choice would be .stylable-foo (I'm always making sure that my component naming is unique). I guess there are a lot of strong personal preferences on this topic (also, CSS naming conventions such as BEM, SMACSS, SUIT...). How can we make sure this is not an obstacle to adoption?
  • Third-party CSS: let's say I build a ViewComponent that depends on a 3rd-party library such as Flickity, so I need to include the library CSS to my component. How would you approach this?
  • Slim templates: I'm a bit concerned about the readability of having div(class="#{styles['foo']} another-class") instead of .stylable-foo.another-class in my Slim templates, but there's probably a way to improve this

# 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

This comment has been minimized.

@Spone

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.


# 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

This comment has been minimized.

@keithamus

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.

This comment has been minimized.

@Spone

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?

This comment has been minimized.

@keithamus

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.

adr/0677-css-encapsulation.md Outdated Show resolved Hide resolved
adr/0677-css-encapsulation.md Outdated Show resolved Hide resolved
adr/0677-css-encapsulation.md Outdated Show resolved Hide resolved
joelhawksley and others added 4 commits Mar 26, 2021
Co-authored-by: Simon Taranto <srt32@github.com>
Copy link
Collaborator

@elia elia left a comment

So happy to finally see this released! 👏
Just a few considerations/needs I would have as a potential user of ViewComponent::Stylable.

How can we support porting existing SCSS code and running tools like postCSS and CSSLint?

Using params to store the list of compiled CSS might not play well with caching. Using a current attribute with a Set in it would probably look better, but I'm not sure it really solves the problem.

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 Foo::BarBaz into .foo--bar-baz. As long as the components are stored in global constants any string derived from the class name should work.

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.

Copy link
Member

@BlakeWilliams BlakeWilliams left a comment

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

This comment has been minimized.

@BlakeWilliams

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.

This comment has been minimized.

@koddsson

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.

This comment has been minimized.

@BlakeWilliams

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. 🤔

lib/view_component/stylable.rb Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@ def compiled?
CompileCache.compiled?(component_class)
end

def compile(raise_errors: false)
def ensure_compiled(raise_errors: false)

This comment has been minimized.

@BlakeWilliams

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)

This comment has been minimized.

@BlakeWilliams

BlakeWilliams Mar 29, 2021
Member

This allows for global styles, can we make this raise to disallow escaping the component scope?

self.class.styles
end
end
end

This comment has been minimized.

@BlakeWilliams

BlakeWilliams Mar 29, 2021
Member

Suggested change
end
end
def commenter_name
commenter.name
def call
content_tag(:div, "Hello, World!", class: styles['foo'])

This comment has been minimized.

@BlakeWilliams

BlakeWilliams Mar 29, 2021
Member

Suggested change
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

adr/0677-css-encapsulation.md Outdated Show resolved Hide resolved

### 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.

This comment has been minimized.

@koddsson

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?

This comment has been minimized.

@Spone

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

This comment has been minimized.

@koddsson

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.

This comment has been minimized.

@koddsson

koddsson Mar 30, 2021
Member

Seems like there's text cut off here @joelhawksley?

Co-authored-by: Kristján Oddsson <koddsson@gmail.com>
@joelhawksley
Copy link
Contributor Author

@joelhawksley joelhawksley commented Apr 8, 2021

@koddsson @BlakeWilliams @jaredcwhite @Spone @elia @keithamus @srt32 thanks for taking the time to review!

Should this be a part of ViewComponent at all?

I can think of some reasons why VC could 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. - @jaredcwhite

I think this is a fair point, Jared. As proposed here, one would need to include the Stylable module, but I'd like to see us offer some sort of built-in solution eventually. We should make it easy to opt out of this behavior.

Class naming conventions

There 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

Using params to store the list of compiled CSS might not play well with caching. Using a current attribute with a Set in it would probably look better, but I'm not sure it solves the problem. - @elia

🤦 I had a feeling that the params approach was a bit naive. Let's remove it for now, then.

How can we support porting existing SCSS code and running tools like postCSS and CSSLint?

I think that autoprefixer-rails might be our ticket here. It already uses PostCSS under the hood, and I'd love to see us come up with a convention-over-configuration solution to this problem. Got any ideas here @palkan?

Ship the collected CSS of all existing components

@elia, you're right. For a lot of applications, this is probably the correct approach.

@palkan
Copy link
Contributor

@palkan palkan commented Apr 9, 2021

Hey everyone!

I think that autoprefixer-rails might be our ticket here.

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 <style> along with the template. That's brilliant.

Should this be a part of ViewComponent at all?

I'm a bit worried about additional runtime dependencies (currently, sass). If we can make this dependency optional (similarly, for example, like Rails does for redis in Action Cable), that would be a good compromise, in my opinion.

However, abstractizing Stylable and #styles also makes sense. The current implementation could be one of the possible adapters. And I already have another one here 🙂.

Class naming conventions

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., <configurable-namespace>-<component_name.parameterize>-<class>.

API for Slim templates

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.

No Webpacker

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 @apply).

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.

@Spone
Copy link
Collaborator

@Spone Spone commented Apr 9, 2021

API for Slim templates

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.

Quick try: Spone/slim#1 (requires some minor changes to the slim parser, see the diff for details)

~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>
@Spone
Copy link
Collaborator

@Spone Spone commented Apr 26, 2021

To follow up on Third-party CSS, here is an example to make it a bit more concrete. Let's consider a GalleryComponent that is a wrapper around Flickity, so the component requires both the JS and CSS from Flickity.

Currently, I see 4 possible approaches:

  • #1 Using the provided CDN obviously works out of the box if you add the <script> and <link> to your layout (but it's up to you to find a way to include them only if the component is used on the page)
  • #2 Install the library through NPM, then require it with the Asset Pipeline:
//= require flickity
  • #3 Same as above but with Webpacker, you would then import the assets in the component JS:
// app/components/gallery_component/gallery_component.js
import Flickity from "flickity";
import "flickity/dist/flickity.css";
  • #4 Finally, you can vendor the assets in your component sidecar folder, but then you have a duplication problem when multiple components depend on the library (and you can't update the library easily).

In approaches #2 and #3, you still need a bundle of JS and CSS alongside your inlined components' CSS (and that's very probably what most projects will have anyway, if they have legacy CSS, or use a CSS reset, default values, variables, utility CSS...).

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 integrity attribute in package-lock.json or yarn.lock could be of some use?)

@Spone Spone mentioned this pull request May 10, 2021
4 of 12 tasks complete
if !rendered_components.include?(rendered_components_identifier)
request.params["___rendered_view_components"] << rendered_components_identifier

content_tag(:style, self.class._css.html_safe)

This comment has been minimized.

@halo

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants