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

sass load order problem after upgrading to 4.0.0.beta8 #597

Open
jcoyne opened this issue Jan 26, 2019 · 14 comments

Comments

@jcoyne
Copy link
Contributor

@jcoyne jcoyne commented Jan 26, 2019

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

This comment has been minimized.

Copy link

@meacuna meacuna commented Mar 15, 2019

We got stuck on the same problem.

Did you manage to get a solution @jcoyne?

@jcoyne

This comment has been minimized.

Copy link
Contributor Author

@jcoyne jcoyne commented Mar 18, 2019

No.

@zarqman

This comment has been minimized.

Copy link

@zarqman zarqman commented May 7, 2019

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 added a commit to carbonfive/raygun-rails that referenced this issue Oct 8, 2019
In sprockets 4.0.0, the `link_directory ../stylesheets .css` directive
causes sprockets to process all `.scss` files in an arbitrary order,
regardless of any `@import` statements. This leads to undefined variable
errors when e.g. the file using a variable is processed before the file
that defines it.

Fix by explicitly rooting the sprockets manifest at `application.css`,
as suggested here:

rails/sprockets#597 (comment)
christiannelson added a commit to carbonfive/raygun-rails that referenced this issue Oct 8, 2019
In sprockets 4.0.0, the `link_directory ../stylesheets .css` directive
causes sprockets to process all `.scss` files in an arbitrary order,
regardless of any `@import` statements. This leads to undefined variable
errors when e.g. the file using a variable is processed before the file
that defines it.

Fix by explicitly rooting the sprockets manifest at `application.css`,
as suggested here:

rails/sprockets#597 (comment)
christiannelson added a commit to carbonfive/raygun-rails that referenced this issue Oct 8, 2019
In sprockets 4.0.0, the `link_directory ../stylesheets .css` directive
causes sprockets to process all `.scss` files in an arbitrary order,
regardless of any `@import` statements. This leads to undefined variable
errors when e.g. the file using a variable is processed before the file
that defines it.

Fix by explicitly rooting the sprockets manifest at `application.css`,
as suggested here:

rails/sprockets#597 (comment)
@mattbrictson

This comment has been minimized.

Copy link
Contributor

@mattbrictson mattbrictson commented Oct 10, 2019

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

dgcliff added a commit to NEU-Libraries/charon that referenced this issue Oct 10, 2019
@jrochkind

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 10, 2019

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

This comment has been minimized.

Copy link

@zarqman zarqman commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 10, 2019

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

This comment has been minimized.

Copy link

@zarqman zarqman commented Oct 10, 2019

@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

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 10, 2019

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

This comment has been minimized.

Copy link

@mockdeep mockdeep commented Oct 12, 2019

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.

jkeck added a commit to sul-dlss/mirador_sul that referenced this issue Oct 14, 2019
@jrochkind

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 15, 2019

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

This comment has been minimized.

Copy link

@zarqman zarqman commented Oct 15, 2019

@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

This comment has been minimized.

Copy link
Contributor

@jrochkind jrochkind commented Oct 15, 2019

Aha, @schneems pointed out that he already valiantly tried to get Rails to switch this in rails/rails#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. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.