-
Notifications
You must be signed in to change notification settings - Fork 137
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
Comments
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 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.
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
We should consider making embroider's built-in style rules extensible. ember-auto-import has A bigger-picture change I'm considering is to actually have a |
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.
Fixed by #1045 (lemme know if I've misinterpreted) |
@rwjblue @simonihmig should we log "happy-path" here? |
I'd like to re-open, as the issue describes a few more problems than what #1045 fixed. We still need that empty
What exactly do you mean by this? |
I mean recommended (+working) way to use sass with embroider |
@lifeart Log = document? |
@lolmaus correct |
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
@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? |
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. |
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. |
@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 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? |
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 Instead, they need to precisely // 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
@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.
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. |
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 For example in your example above, I believe On the positive side, they seem to have planned to adopt Sass modules for v6: twbs/bootstrap#29853
Well, if we import Bootstrap from our 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? |
Yes, but then you can't use any bootstrap mixins or variables in your own scss.
Correct. This is a problem for everybody using module-oriented bundlers (webpack, rollup, parcel, vite, snowpack, etc).
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:
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:
|
Are you sure? One of key features of
Dart Sass is the only modern, maintained implementation of Sass.
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.
Dart Sass is a drop-in replacement for Converting from Ruby Sass to |
@ef4 @simonihmig BTW, how does minification of HTML classes fit into all this? |
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.
Ok, that's good, so at least people shouldn't be using that as a reason not to adopt sass modules. |
We've currently planning to remove SCSS in favor of vanilla CSS and 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) |
@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/ https://w3techs.com/technologies/history_overview/css_framework/all 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. |
😄 |
Approach number 2. seems to be working... ish Some things I learned along the way:
|
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. |
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 issass-loader
(andsass
itself),mini-css-extract-plugin
andcss-loader
. The latter two are already used by@embroider/webpack
, but I made them also dependencies of the app itself. I chose to usemini-css-extract-plugin
instead the usually proposedstyle-loader
also in development, as I wanted the app to also work in FastBoot (which seems to not work withstyle-loader
, as it usesdocument
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
tostyles/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 indist/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 importedapp.scss
(moved from/app/styles/app.scss
to/app/app.scss
, as otherwise it would not find it somehow) fromapp.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:
/app/styles/app.css
, which is a bit confusing. Otherwise the build crashes withCould 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
#1045Reproduction
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:
You see here:
<link>
anymore, but a script containing webpack loader code (includingstyle-loader
that again breaks in FastBoot!)vendor.css
is not even loaded in the browser/assets/chunk.3dc0c8a50b82bb3a4c50.css
is the compiled Sass, and it gets applied correctly!Reproduction
The text was updated successfully, but these errors were encountered: