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

Embroider-native support for Sass #1033

Open
simonihmig opened this issue Nov 30, 2021 · 24 comments
Open

Embroider-native support for Sass #1033

simonihmig opened this issue Nov 30, 2021 · 24 comments

Comments

@simonihmig
Copy link
Collaborator

simonihmig commented Nov 30, 2021

I played a bit trying to make an Embroider app work with Sass natively, i.e. without using ember-cli-sass, as that seems to be the new way to integrate new tools to the build-pipelines (see #916 (comment)). However that turned out to be not that trivial as I hoped.

I tried the following three approaches, no one worked really perfectly. Wonder what the "right" way would be, and how to fix its issues?

For all approaches, I installed the related webpack plugins, and added their config to ember-cli-build.js. So this is sass-loader (and sass itself), mini-css-extract-plugin and css-loader. The latter two are already used by @embroider/webpack, but I made them also dependencies of the app itself. I chose to use mini-css-extract-plugin instead the usually proposed style-loader also in development, as I wanted the app to also work in FastBoot (which seems to not work with style-loader, as it uses document APIs at runtime).

I have reproductions for all these approaches at https://github.com/simonihmig/embroider-sass-app in different branches.

1. Rename app.css to app.scss

The obvious and naive approach was to just rename styles/app.css to styles/app.scss and hope that it would just work. It didn't.

It seems it doesn't even try to compile the sass. app.scss is found in dist/assets/ in its original form. The stylesheet reference in index.html points to a now nonexisting file (/assets/embroider-sass-app.css)

Reproduction

2. Import app.scss from app.js

Similar to what the sass-loader itself suggests and what you mostly see in "webpack-native" projects AFAIK, I imported app.scss (moved from /app/styles/app.scss to /app/app.scss, as otherwise it would not find it somehow) from app.js.

This does work in general, the Sass is transformed, and you see the compiled CSS linked and used. However it is not quite ideal:

  • you seem to need an empty /app/styles/app.css, which is a bit confusing. Otherwise the build crashes with Could not find app css: "/assets/embroider-sass-app.css" in index.html. Found the following instead: - /assets/vendor.css
  • the <link> tag is added at the end of the index.html (alongside the scripts). For a "regular" SPA this is probably ok-ish, but for a SSR app (FastBoot) this causes a FOUC.
    Fixed by Append styles imported in JS to end of document.head #1045

Reproduction

3. Use index.html as the entrypoint

AFAIU Embroider itself follows he approach of using HTML as the source of truth, the root of its dependency tree. So I tried to follow that idea, by adding a separate <link integrity="" rel="stylesheet" href="{{rootURL}}app.scss"> to index.html.

For this to (somehwat) work I had to patch @embroider/webpack to also add the HTML's styles as entrypoints to its webpack config.

Don't know for sure if this reasonable at all. But it does work for the Sass part at least. However it also messes things up related to the static CSS. This is what the built index.html part looks like then:

<script rel="stylesheet" href="/assets/vendor.css" src="/assets/chunk.4cf6812ef5b6b3308bec.js"></script>
   
<script rel="stylesheet" href="/assets/embroider-sass-app.css" src="/assets/chunk.eac6d09440173884bc29.js"></script>

<link href="/assets/chunk.3dc0c8a50b82bb3a4c50.css" rel="stylesheet">
<script integrity="" rel="stylesheet" href="/app.scss" src="/assets/chunk.3dc0c8a50b82bb3a4c50.js"></script>

You see here:

  • the (static) CSS files don't have a <link> anymore, but a script containing webpack loader code (including style-loader that again breaks in FastBoot!)
  • which means vendor.css is not even loaded in the browser
  • the /assets/chunk.3dc0c8a50b82bb3a4c50.css is the compiled Sass, and it gets applied correctly!
  • there is a accompanying script file to that Sass file, which seems to contain only useless empty webpack module definitions

Reproduction

@ef4
Copy link
Contributor

ef4 commented Nov 30, 2021

Thanks for working on this.

I think the main feature we will need is the ability to disable the classic CSS support for the app (while keeping it for any addons that are relying on it). That is what is forcing app/styles/app.css to exist. I think this is also why you couldn't keep your scss in app/styles -- that path gets enough legacy handling that it doesn't just flow through normally to where webpack can see it.

I think both option 2 and option 3 are valid strategies that we should support.

We deliberately said in the embroider spec that we support importing CSS into JS as a way for a JS module to express a dependency on the CSS. It's more precise than importing all your CSS at once because it allows the CSS to split and lazy-load along with the JS. Adding sass loader to that should necessarily also make importing SCSS into JS work.

And using a CSS entry point from the HTML also makes a lot of sense, for things controlled by the app that it always wants to load. Especially things like CSS resets that should come before anything else. Within the module graph you don't get very strong guarantees of evaluation order, so the HTML is a better place for that.

the tag is added at the end of the index.html (alongside the scripts). For a "regular" SPA this is probably ok-ish, but for a SSR app (FastBoot) this causes a FOUC.

This seems like a thing we should fix, even without SCSS when you import CSS you can get style chunks and they will get the same treatment. We can probably always insert them at the end of <head> instead. We'd probably want to maintain the same relative order between all style chunks though.

The latter two are already used by @embroider/webpack, but I made them also dependencies of the app itself.

We should consider making embroider's built-in style rules extensible. ember-auto-import has styleLoaderOptions and cssLoaderOptions here. We could do a similar thing for @embroider/webpack.

A bigger-picture change I'm considering is to actually have a webpack.config.js in the app, which imports utilities from Embroider to setup the standard things we need. Stylistically it would be similar to the way that v2 addons can use a rollup config where we've provided standard utilities that take care of most of the complexity, without preventing you from adding other things. This would help with the extensibility if we did it well.

@lolmaus
Copy link
Collaborator

lolmaus commented Dec 3, 2021

simonihmig added a commit to simonihmig/embroider that referenced this issue Dec 10, 2021
This addresses the issue raised in embroider-build#1033 ("2. Import app.scss from app.js") that link tags added through imports of CSS in JS get added next to the script entrypoint (at the body's end) rather than to the document's head, causing a FOUC in FastBoot rendered pages.
@rwjblue
Copy link
Collaborator

rwjblue commented Dec 13, 2021

Fixed by #1045 (lemme know if I've misinterpreted)

@rwjblue rwjblue closed this as completed Dec 13, 2021
@lifeart
Copy link
Collaborator

lifeart commented Dec 13, 2021

@rwjblue @simonihmig should we log "happy-path" here?

@simonihmig
Copy link
Collaborator Author

I'd like to re-open, as the issue describes a few more problems than what #1045 fixed. We still need that empty app.css, and the option "3. Use index.html as the entrypoint" is not working at all.

should we log "happy-path" here?

What exactly do you mean by this?

@simonihmig simonihmig reopened this Dec 13, 2021
@lifeart
Copy link
Collaborator

lifeart commented Dec 13, 2021

I mean recommended (+working) way to use sass with embroider

@lolmaus
Copy link
Collaborator

lolmaus commented Dec 13, 2021

@lifeart Log = document?

@lifeart
Copy link
Collaborator

lifeart commented Dec 13, 2021

@lolmaus correct

@simonihmig
Copy link
Collaborator Author

Ok, yeah, we definitely should do that. But so far I am not done with my exploration of that happy path. Approach 2 still requires that empty app.css. And I found new issues when using FastBoot here: #1049

I think the main feature we will need is the ability to disable the classic CSS support for the app (while keeping it for any addons that are relying on it).

@ef4 I have been able to provide some contributions which only had to touch small (contained) pieces of the puzzle, but I am not really able yet to see the big picture of how all pieces that constitute Embroider play together, at a detailed technical level at least. So would you be able to provide an outline of what needs to happen at which place to implement this feature?

@ef4
Copy link
Contributor

ef4 commented Dec 22, 2021

After digging deeper into the state of sass, making it really nice is a bigger pain than I recognized. We can definitely maintain compatibility with how things work in ember-cli-sass, but we can't easily do a lot better, in terms of build performance and code splitting.

The problem is that sass isn't just a transpiler. It's a bundler in its own right. That was necessary in the era when Sass was conceived. But I would argue that it has become a liability, and it means Sass doesn't really play well with whole-app optimization that involves your JS and styles together.

Components are the bedrock primitive of modern Ember apps. I believe that styles should be oriented around components, such that there's an obvious and automatic one-to-one mapping between components and their stylesheets, and that if we've optimized away loading a component we should obviously optimize away loading its styles too. Sass is really not equipped to play nice in that scenario.

The most relevant discussion on the Sass language repo I've found is sass/sass#2873.

@simonihmig
Copy link
Collaborator Author

Components are the bedrock primitive of modern Ember apps. I believe that styles should be oriented around components, such that there's an obvious and automatic one-to-one mapping between components and their stylesheets, and that if we've optimized away loading a component we should obviously optimize away loading its styles too. Sass is really not equipped to play nice in that scenario.

I certainly agree with that. But also I don't quite understand what the problem is you see here. Could you elaborate how you think Sass is not working well here?

For the current experimentation I started with one of our apps (our own homepage actually), things are working quite ok now (with the latest fixes you just released). Our setup is as follows:

  • we import app.scss in my app.ts, which contains only "global" styles (resets and styles for generic HTML elements)
  • each component has its own colocated my-component.scss. Previously we would have imported all these component styles from our global app.scss. Now with the webpack-native approach, each component JS imports its own styles.
  • we also have some "page-level" Sass here and there that is imported from the route.
  • note that we don't use the classic @import, rather Sass-modules using @use. So wherever we need variables or mixins, we pull those in explicitly, instead of relying on things to be globally defined. Maybe that helps here, to not pull in (bundle) unneeded stuff for component styles?

With route-based code splitting, I see those CSS chunks lazy loaded, and they only contain what they should (lazy component CSS and eventually page-level CSS, see above)!

Where I do see significant problems is when adding FastBoot to the game, but that's a different story, see #1049.

@lolmaus
Copy link
Collaborator

lolmaus commented Dec 22, 2021

@ef4 Do you imply that modern, Embroider-driven Ember CLI will not support for Sass in its current state?

This means that developers will have to either stay on legacy, non-Embroider ember-cli-sass forever or to resort to orchestrators like Grunt or Gulp in order to wrap separate build pipelines of Ember CLI and Sass and then merge results in the dist/ folder...

My next question is purely hypothetical because we and many other developers are bound to Sass due to being heavily invested into Bootstrap, Foundation, Bulma, UIKit, Cirrus, etc, so switching a CSS preprocessor is not an option. But anyway, what other CSS preprocessors is Embroider gonna support? Or does Embroider enforce shifting the CSS paradigm from semantic to presentational (e. g. Tailwind), or reverting plain old CSS, giving up on variables, mixins, loops, conditionals, selector nesting, media query bubbling, etc?

@ef4
Copy link
Contributor

ef4 commented Dec 22, 2021

note that we don't use the classic @import, rather Sass-modules using @use.

Yes, this is pretty critical. I get the impression it's not the common way most existing apps use Sass, and it's a nontrivial refactor. So what I was saying above is that I think most apps won't get out-of-the-box compatibility with their Sass that allows whole-app optimization. I do think it's probably possible to refactor the Sass into that explicit pattern.

For example, if you look at the bootstrap sass docs, they use only @import. We'll need to teach people not to do that, and if they don't learn that, they might not realize they're getting separate copies of all of bootstrap all over their output.

Instead, they need to precisely @use the specific sub-parts of bootstrap in specific SCSS files, like:

// before
div {
  @include .media-breakpoint-down(sm) {
    margin-top: 64px;
    color: $link-hover-color;
  }
}

// after
@use "ember-bootstrap/bootstrap/scss/mixins/breakpoints";
@use "ember-bootstrap/bootstrap/scss/variables";

div {
  @include breakpoints.media-breakpoint-down(sm) {
    margin-top: 64px;
    color: variables.$link-hover-color
  }
}

If you refactor your sass like this, I think you can get route splitting without excessive duplication, and it sounds like @simonihmig has proved that out. The hardest part will be if any third-party stuff doesn't keep their mixins & variables cleanly separate from their styles, because then you're out of luck and forced to either not use their mixins & variables or get duplication of their styles per component that uses them.

What do Sass users think of adopting this stricter style? I don't have a good sense for where Sass as a language is heading and whether there's any community consensus toward more explicit use of "sass modules" like this.

The part we can't really improve is that, from Sass's perspective, every SCSS file that you import into a component is a totally separate build that shares no work with every other file. So every time a new component does @use "ember-bootstrap/bootstrap/scss/variables";, Sass re-parses that file from scratch. And you must use the Dart Sass implementation (it's the only one that supports @use), which I believe is considered the slower implementation. So it's going to be somewhat inefficient, and adopting the stricter style with sass modules makes this problem worse.

Do you imply that modern, Embroider-driven Ember CLI will not support for Sass in its current state?

@lolmaus definitely not. What I'm saying is that "its current state" is what you might be stuck with, rather than something better integrated with the app build.

or to resort to orchestrators like Grunt or Gulp

With ember-cli-sass you already get a non-integrated build that gets the same results you'd get with grunt or gulp. What I'm trying to articulate is that thinking of "the sass build" as a separate step that can happen outside the Javascript build means it can never take advantage of knowledge about the structure of the app. Sass is designed around the assumption that it governs its own separate build, and it doesn't have a concept like ECMA dynamic import that would let us map our app analysis into sass.

@simonihmig
Copy link
Collaborator Author

Thanks for the detailed explanation, that makes a lot of sense.

As you mentioned Bootstrap, we indeed have a (different) project where we use Bootstrap Sass (using @import), while still writing our own custom styles using @use. We tried to import Bootstrap's files using @use, but IIRC that mostly failed.

For example in your example above, I believe @use "ember-bootstrap/bootstrap/scss/mixins/breakpoints"; will actually fail, as the mixins themselves refer to Sass variables (for e.g. default arguments), which are not available as globals when they haven't been @imported before.

On the positive side, they seem to have planned to adopt Sass modules for v6: twbs/bootstrap#29853

We'll need to teach people not to do that, and if they don't learn that, they might not realize they're getting separate copies of all of bootstrap all over their output.

Well, if we import Bootstrap from our app.scss as "global styles", and not import anything from it from lazy CSS chunks (component styles), then this should work, without any duplication, right?

However at this point we are not talking about Embroider-specific problems anymore, e.g. a React app using webpack and Sass would suffer from the same problems, right?

@ef4
Copy link
Contributor

ef4 commented Dec 22, 2021

Well, if we import Bootstrap from our app.scss as "global styles", and not import anything from it from lazy CSS chunks (component styles), then this should work, without any duplication, right?

Yes, but then you can't use any bootstrap mixins or variables in your own scss.

However at this point we are not talking about Embroider-specific problems anymore, e.g. a React app using webpack and Sass would suffer from the same problems, right?

Correct. This is a problem for everybody using module-oriented bundlers (webpack, rollup, parcel, vite, snowpack, etc).

On the positive side, they seem to have planned to adopt Sass modules for v6

That's good, but this does indicate that there are blockers out in the ecosystem that will make this a slower transition. I especially wonder about this comment on the issue you linked:

the module system only works in Dart Sass, not in ruby Sass or Lib Sass. Since the majority of build systems won't support this yet, I guess it won't be a good idea to switch now.

As far as I can tell, this remains true two years later. Which is going to further slow the Sass ecosystem's adoption of sass modules.

@simonihmig
Copy link
Collaborator Author

As far as I can tell, this remains true two years later. Which is going to further slow the Sass ecosystem's adoption of sass modules.

But isn't everybody using dart already, or at least supposed to switch?

According to node-sass:

Warning: LibSass and Node Sass are deprecated. While they will continue to receive maintenance releases indefinitely, there are no plans to add additional features or compatibility with any new CSS or Sass features. Projects that still use it should move onto Dart Sass.

@lolmaus
Copy link
Collaborator

lolmaus commented Dec 22, 2021

every SCSS file that you import into a component is a totally separate build that shares no work with every other file. So every time a new component does @use "ember-bootstrap/bootstrap/scss/variables";, Sass re-parses that file from scratch

Are you sure? One of key features of @use is that "using" the same module multple times will not emit CSS more than once. This means that Sass somehow remembers which modules were used, which in turn is very close to caching of used modules.


And you must use the Dart Sass implementation (it's the only one that supports @use), which I believe is considered the slower implementation.

Dart Sass is the only modern, maintained implementation of Sass. libsass/sassc is deprecated, and Ruby Sass is long dead. Dart was chosen specifically because it provides high-level convenience without compromising speed. I believe Dart Sass is considered a fast implementation in the community.


Yes, but then you can't use any bootstrap mixins or variables in your own scss.

Variables are defined in a separate file locally, and that file can be imported as a module.

As for mixins and functions, that's true and it's a huge pain of the transtion period.


the module system only works in Dart Sass, not in ruby Sass or Lib Sass. Since the majority of build systems won't support this yet, I guess it won't be a good idea to switch now.

As far as I can tell, this remains true two years later. Which is going to further slow the Sass ecosystem's adoption of sass modules.

Dart Sass is a drop-in replacement for libsass. E. g. ember-cli-sass was upgraded to Dart Sass transparently, through node-sass chagning its binary dependency from libsass to Dart Sass.

Converting from Ruby Sass to libsass was indeed an issue, because Ruby Sass was effectively using Compass as a build pipeline, so upgrading required switching from Ruby to Node or something (unless the rest of your ecosystem was Ruby, in which case you would probably prefer to use binary dependency on libsass within Ruby). But converting from libsass to Dart Sass in the Node realm is a non-issue.

@lolmaus
Copy link
Collaborator

lolmaus commented Dec 22, 2021

@ef4 @simonihmig BTW, how does minification of HTML classes fit into all this?

@ef4
Copy link
Contributor

ef4 commented Dec 22, 2021

This means that Sass somehow remembers which modules were used

This is the crux of what's problematic in the Sass compiler's current architecture: that's true within one build, but from Sass's perspective every Ember component with a corresponding SCSS file is a separate build. It has no way to share state between them. It considers every one a distinct entrypoint.

LibSass and Node Sass are deprecated

Ok, that's good, so at least people shouldn't be using that as a reason not to adopt sass modules.

@sandstrom
Copy link
Contributor

We've currently planning to remove SCSS in favor of vanilla CSS and postCSS or lightningCSS for simple pre-processing (imports, automatic vendor-prefixing and a few other minor things).

Just one data point, but my guess is that SCSS will gradually become less popular, and with it the need for native support in Embroider will fade. Maybe the effort is better spent elsewhere?

(I know people in this thread will disagree, because you are discussing this precisely because you want Sass, but overall I think usage is decreasing)

@vlascik
Copy link
Contributor

vlascik commented Feb 23, 2023

@sandstrom As long as Bootstrap is in SASS, and pretty much every other website is done with Bootstrap, SASS is not going anywhere.

https://2022.stateofcss.com/en-US/css-frameworks/
Bootstrap usage: over 80% of respondents are using or used Bootstrap. Their websites have to be maintained.

https://w3techs.com/technologies/history_overview/css_framework/all
According to this 19% of all websites on the internet have Bootstrap, and that number is probably so low only because their detection is fairly bad (80% of what they can detect is Bootstrap, and probably excludes custom builds).

Also, if I had to predict the future, if you're still writing CSS by hand in 2023, you're probably doing it wrong anyway - thanks to utility class frameworks like Tailwind and Bootstrap as well, and WYSIWYGs where you visually design by just adding/removing CSS classes in UI, writing tens of thousands of lines of HTML and custom CSS is just a waste of time. Bootstrap is ready for it too, and thanks to that, probably still has years and years of life in it, regardless of what overcomplicated approach is all the rage this week.

@sandstrom
Copy link
Contributor

sandstrom commented Feb 24, 2023

"Just one data point"

😄

@amk221
Copy link

amk221 commented Jan 22, 2024

Approach number 2. seems to be working... ish

Some things I learned along the way:

  • It outputs: <link rel="stylesheet" href="assets/my-app.css"> does not exist on disk. If this is intentional, use a data-embroider-ignore attribute.

    ...and if you follow those instructions then you end up with a dud link tag

  • It leaves *.scss files in the dist directory, with seemingly no way to get rid of them using webpack

  • It outputs the css file as dist/vendors-assets_my-app_js.css for dev builds and dist/assets/my-app.js.css for prod builds, not sure why it's a different path.

  • api: 'modern' does not work if you import from node modules like @use '@my-scope/some-scss'

  • production builds end up with duplicate stylesheet link tags, one called app.css and called chunk....css

@kategengler
Copy link

I'm also finding with approach number 2 that we lose hot-reloading of styles as they change (getting a full reload with every change of an scss file), which makes doing a lot of style work a chore.

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

No branches or pull requests

9 participants