embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
335 stars 136 forks source link

Embroider-native support for Sass #1033

Open simonihmig opened 2 years ago

simonihmig commented 2 years ago

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 https://github.com/embroider-build/embroider/issues/916#issuecomment-903175051). 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:

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:

Reproduction

ef4 commented 2 years ago

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 commented 2 years ago

@simonihmig Can this help?

https://github.com/peopledoc/ember-auto-import-chunks-json-generator

rwjblue commented 2 years ago

Fixed by https://github.com/embroider-build/embroider/pull/1045 (lemme know if I've misinterpreted)

lifeart commented 2 years ago

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

simonihmig commented 2 years ago

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?

lifeart commented 2 years ago

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

lolmaus commented 2 years ago

@lifeart Log = document?

lifeart commented 2 years ago

@lolmaus correct

simonihmig commented 2 years ago

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 commented 2 years ago

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 https://github.com/sass/sass/pull/2873.

simonihmig commented 2 years ago

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:

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/twbs/bootstrap/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ef4 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

"Just one data point"

😄

amk221 commented 8 months ago

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

Some things I learned along the way:

kategengler commented 3 weeks ago

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.