sass load order problem after upgrading to 4.0.0.beta8

jcoyne commented 5 years ago

Setup:

I have three scss files in app/assets/stylesheets

# app/assets/stylesheets/application.scss
@import 'variables';
@import 'one';

# app/assets/stylesheets/variables.scss
$color-cardinal-red: #8c1515;

# app/assets/stylesheets/one.scss
p { background-color: $color-cardinal-red }

Expected behavior

I expect that ./bin/rails assets:precompile compiles the application.scss (just as sprockets 3.x did)

✨  Done in 0.07s.
I, [2019-01-26T00:08:08.492689 #126]  INFO -- : Writing /Users/jcoyne85/workspace/sul-dlss/testapp/public/assets/application-c2183ea8f9e0179587d76b296815b5607624492b750c23a7597b228dadb6c163.js
I, [2019-01-26T00:08:08.493084 #126]  INFO -- : Writing /Users/jcoyne85/workspace/sul-dlss/testapp/public/assets/application-c2183ea8f9e0179587d76b296815b5607624492b750c23a7597b228dadb6c163.js.gz
I, [2019-01-26T00:08:08.509326 #126]  INFO -- : Writing /Users/jcoyne85/workspace/sul-dlss/testapp/public/assets/application-1d5359781b5a578fdd5946f50677ccbe14b2ab4a0a4235c05a4e17522fa7c02c.css
I, [2019-01-26T00:08:08.509417 #126]  INFO -- : Writing /Users/jcoyne85/workspace/sul-dlss/testapp/public/assets/application-1d5359781b5a578fdd5946f50677ccbe14b2ab4a0a4235c05a4e17522fa7c02c.css.gz

Actual behavior

rails aborted!
Sass::SyntaxError: Undefined variable: "$color-cardinal-red".
/Users/jcoyne85/workspace/sul-dlss/testapp/app/assets/stylesheets/one.scss:1
Tasks: TOP => assets:precompile

System configuration

Sprockets version: 4.0.0.beta8
Ruby version: 2.53

Example App

https://github.com/jcoyne/testapp

meacuna commented 5 years ago

We got stuck on the same problem.

Did you manage to get a solution @jcoyne?

jcoyne commented 5 years ago

No.

zarqman commented 5 years ago

I found I had to modify app/config/manifest.js by removing the //= link_directory ../stylesheets .css line and replacing it with //= link application.css.

FYI, it turns out //= link_directory ../stylesheets .css also include all .scss and .sass files, which causes all CSS files to be processed independently, separate from any @import ... which is how the variables end up missing.

In case it helps, here's my complete manifest.js:

//= link_tree ../images
//= link application.css

I'm using webpacker for JS, so it's not part of my Sprockets config. You might still need a line for such (although there too, you might want just //= link application.js.

Hint: Sprockets is overly generous when identifying directives and has a bad habit of ignoring what you think might be a commented out directive. If in doubt, delete old lines entirely.

mattbrictson commented 4 years ago

The comment by @zarqman fixes the issue, at least in my case. I think this can be closed.

jrochkind commented 4 years ago

So, I don't understand the effects of changing manifest.js the way @zarqman suggests, I don't know if it will cause other problems. I'm reluctant to change config/maninfest.js to something other than what Rails generated for me, without understanding what I'm doing.

I believe Rails 5.2 generates a config/manifest.js that looks like this:

//= link_tree ../images
//= link_directory ../javascripts .js
//= link_directory ../stylesheets .css

And Rails 6.0 generates a config/manifest.js that looks like this, just leaving out javascripts since Rails 6 doesn't have sprockets handle javascripts.

//= link_tree ../images
//= link_directory ../stylesheets .css

If Rails is generating a config/manifest.js that doesn't work properly, that is a bug with something. But I'm not sure if the fix is getting Rails to generate a different config/manifest.js, or if that generated one is supposed to work with sprockets 4.0.

I think this problem is actually going to trip up a lot of people now that sprockets 4.0.0 final is released. It seems to be doing something... different and backwards incompatible (and perhaps buggy?) than sprockets 3.0, which doesn't seem to be documented... and may not be intended? And may effect anyone who is trying to use a sass variable defined in a file.... elsewhere? in a gem?... and is depending on things to be loaded in the order of sprockets requires? Not totally sure what's going on.

@schneems

jrochkind commented 4 years ago

Hmm, the example given in https://github.com/rails/sprockets/blob/master/UPGRADING.md for an app/assets/config/manifest.js indeed does use //= link application.js and not a link_tree directive.

I wonder if the bug really is that Rails is generating a manifest.js that is not what Sprockets expects/recommends? And that works differently in sprockets 4 and 3 for some reason? And a bug/PR should be filed with Rails?

Def could use some advice from @schneems

zarqman commented 4 years ago

I forgot to link to it previously, but I provided more background on this Rails issue: rails/rails#36204.

In short, application.css already contains require_tree ... which reads the entire tree.

Adding link_directory ... to manifest.js basically duplicates this work. At best, this is a waste.

At worst, if application.css has changed or removed the default require_tree directives (for example, when dealing with SCSS mixins or variables in separate files that must be loaded in a specific order), then link_directory effectively preempts those changes and can break the SCSS compile.

It may be useful for someone to open a new bug/PR on Rails, since Rails 6.0 and Sprockets 4.0 have now both been released.

jrochkind commented 4 years ago

My understand of link and link_directory in the manifest were they were about creating multiple "top-level" sprockets files, that each thing targetted by a link or link_directory would become a top-level compiled file.

Which, if so, explains why it would break things -- it's trying to take individual partials meant for compilation only in context with other things (like other files that define variables), and trying to compile them individually.

It would not explain why it worked with sprockets 3 though. Or why Rails thought this was the proper thing to generate.

If it is about top-level targets, it is doing a different thing than require/require_tree are though, since those are about including things into the current 'target'.

I am very confused about what's going on though, and need docs and/or explanation from maintainer.

zarqman commented 4 years ago

@jrochkind, yes, my understanding of link, link_directory, and the issue with partials is about the same as yours.

I don't specifically recall what changed between Sprockets 3 & 4 (I vaguely remember some kind of conditional somewhere regarding manifest.js), but I will note that in Rails 5.2 and prior, manifest.js wasn't actually used. Instead, Rails directly referenced application.css + application.js, even though rails new did generate manifest.js.

As of version 6, Rails now uses manifest.js by default--including any copy generated by an earlier version of Rails.

jrochkind commented 4 years ago

Ah, Rails generated an app/config/manifest.js but didn't actually use it prior to Rails 6? (Why was it generating it, just to be future-proof? Maybe I can try to go look for the Rails commit that generated it and see if there's any context, if we can't get context from those who were there at the time).

In Rails 6, does it use it with both sprockets 3 and 4 though?

Actually I have a Rails 5.2 which does seem to use it once I upgrade to Sprockets 4 -- cause that's what produces the error! So maybe Rails 5.2 with sprockets 4 uses it, but Rails 5.2 with sprockets 3 doesn't.

Still very confused. We can work around it in a "cargo cult" way, but I'd like to actually understand what's going on to hopefully contribute a proper fix and document it correctly.

mockdeep commented 4 years ago

Thanks @zarqman, that's very helpful. link_directory compiles all files, non-recursively in the given directory so that they can each be independently included via stylesheet_link_tag. However, in my case and others here, most of the top level files in assets/stylesheets cannot stand alone because they depend on some other variables file being imported first.

It looks like, based on what @zarqman mentioned, Rails used the setting Rails.application.config.assets.precompile for deciding which files to compile from the top level, defaulting to application.css for styles.

The solution is to either, as mentioned previously, update manifest.js to //= link application.css or to make sure all partial scss files are nested in sub-directories.

jrochkind commented 4 years ago

I have spent some time trying to make sure I understand exactly what's going on and have written it up on my blog.

I will try to take what I've learned and turn it into a PR for the upgrading guide
I think that the Rails-generated manifest.js that includes //= link_directory ../stylesheets .css is a mistake, and it would be better and less disruptive as //= link application.css.
- Every current Rails release generates a Gemfile which does not allow sprockets 4 (or use config/manifest.js) without a manual change; if there is a consensus that the generated manifest.js should be different, it would be good to change it before a Rails release that will use it out of the box.
- I can't figure out what code is generating the manifest.js to make a PR though! Or how to get any committer to pay attention to such a PR anyway. :(
I think that existing Rail 5.2.3 or 6.0.0 apps enabling sprockets 4 will probably want to change their manifest.js as above, at least to start with.

Feedback from @schneems on any of this would be awesome.

zarqman commented 4 years ago

@jrochkind, I think these two might be what you're looking for:

railties/lib/rails/generators/rails/plugin/templates/rails/engine_manifest.js.tt
railties/lib/rails/generators/rails/plugin/templates/rails/dummy_manifest.js.tt

jrochkind commented 4 years ago

Aha, @schneems pointed out that he already valiantly tried to get Rails to switch this in https://github.com/rails/rails/pull/28430 but it was rejected. So @zarqman, your rails/rails#36204 is actually maybe a dup of that. :(

I think they were wrong, and it makes upgrading to sprockets 4 significantly more confusing, and I don't think they considered this than rejecting it. While I think it's not too late, I'm not sure if it will be possible to convince Rails maintainers otherwise.

I think this is all ends up very confusing for the user. But will try to at least submit a PR to the Sprockets 4 upgrade guide trying to clear it up -- it takes a lot more words to explain with Rails choice to use link_directory. :(

rails / sprockets