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

change is not detected in @import css file #90

Closed
equivalent opened this issue Apr 29, 2022 · 22 comments
Closed

change is not detected in @import css file #90

equivalent opened this issue Apr 29, 2022 · 22 comments

Comments

@equivalent
Copy link

If I understood correctly the idea of Portshaft (pls correct me if I'm wrong ) then the way how to include css files would be:

in app/assets/application.css

/* Application styles */
@import url('/bootstrap.css');
@import url('/my-custom.css');

in app/assets/bootstrap.css I have the content of bootstrap.css from Bootstrap 5 download page

in app/assets/my-custom.css:

p {
  color: red;
}

my app/views/layout/application.css

<!DOCTYPE html>
<html>
  <head>
    <title>Myapp</title>
    <meta name="viewport" content="width=device-width,initial-scale=1">
    <%= csrf_meta_tags %>
    <%= csp_meta_tag %>

    <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
    <%= javascript_importmap_tags %>
  </head>

  <body>
    <h1>Propshaft test</h1>

    <div class="text-primary">
      If this text is blue, Bootstrap5 css works via Portshaft
    </div>

    <p>If this text is in color then @include css custom file works</p>
  </body>
</html>

So far so good, I run rails server and load the localhost:3000

Screenshot 2022-04-29 at 15 22 55

you can see the <p> is in red

Screenshot 2022-04-29 at 15 24 26

Now I change the app/assets/my-custom.css:

p {
  color: yellow;
}

I save the file and reload my localhost:3000

Screenshot 2022-04-29 at 15 25 26

Screenshot 2022-04-29 at 15 26 01

As you can see the change that I want the collor of <p> to be yellot was not picked up.

Restarting of rails server don't help, only thing that helps is if I rename the file e.g. from app/assets/my-custom.css to app/assets/my-custom-xx.cssand alter the app/assets/application.css`

/* Application styles */
@import url('/bootstrap.css');
@import url('/my-custom-xx.css');

now it the

is yellow

Screenshot 2022-04-29 at 15 34 17

  • this is fresh Rails 7.0.2.4 & Ruby 3.1.1 project generated with rails new myapp -a propshaft
  • no I didn't run rails assets:precompile
rails   assets:reveal
bootstrap.css
application.css
my-custom-xx.css
my-custom.css
stimulus.min.js
stimulus.js
stimulus.min.js.map
stimulus-importmap-autoloader.js
stimulus-autoloader.js
stimulus-loading.js
turbo.min.js.map
turbo.js
turbo.min.js
es-module-shims.js.map
es-module-shims.js
es-module-shims.min.js
actiontext.js
trix.js
trix.css
action_cable.js
actioncable.esm.js
actioncable.js
activestorage.esm.js
activestorage.js
rails-ujs.js
application.js
controllers/hello_controller.js
controllers/index.js
controllers/application.js
@equivalent
Copy link
Author

equivalent commented Apr 29, 2022

I think the issue is with the digested application.css not being notified that there was a change in my-custom.css

that means css_asset_urls compiler never get's triggered when change happens in my-custom.css

one way how I can enforce this is to place some random css to application.css

e.g.

/* Application styles */
@import url('/bootstrap.css');
@import url('/my-custom.css');
blink { 
  color: "red";
}

@brenogazzola
Copy link
Collaborator

I think the root cause is in this comment "Restarting of rails server don't help".

If restarting the server helped, than that would mean the error was that Propshaft was not noticing the file being changed and reloading it. Since restarting the server did not work it means that the file that Propshaft sees as my-custom did not actually change.

Looking at your assets reveal, I see both my-custom.css and my-custom-xx.css. Yet you said you renamed the file (not copied). So it seems that somewhere on your load paths there is another my-custom.css, and that's the file that Propshaft is using.

Instead of rails assets:reveal try using rails assets:reveal:full to get the full path to the files (and not the logical path). Then delete that second my-custom.css.

@equivalent
Copy link
Author

equivalent commented Apr 29, 2022

So it seems that somewhere on your load paths there is another my-custom.css, and that's the file that Propshaft is using.

I've double checked that and seems no my-custom.css is there only once. here is a link to that dummy project https://github.com/equivalent/propshaft-demo-app .

try using rails assets:reveal:full

rake assets:reveal:full
/Users/t/git/myapp/app/assets/stylesheets/bootstrap.css
/Users/t/git/myapp/app/assets/stylesheets/application.css
/Users/t/git/myapp/app/assets/stylesheets/my-custom.css
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus.min.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus.min.js.map
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus-importmap-autoloader.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus-autoloader.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/stimulus-rails-1.0.4/app/assets/javascripts/stimulus-loading.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/turbo-rails-1.0.1/app/assets/javascripts/turbo.min.js.map
/Users/t/.rvm/gems/ruby-3.1.1/gems/turbo-rails-1.0.1/app/assets/javascripts/turbo.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/turbo-rails-1.0.1/app/assets/javascripts/turbo.min.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/importmap-rails-1.0.3/app/assets/javascripts/es-module-shims.js.map
/Users/t/.rvm/gems/ruby-3.1.1/gems/importmap-rails-1.0.3/app/assets/javascripts/es-module-shims.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/importmap-rails-1.0.3/app/assets/javascripts/es-module-shims.min.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actiontext-7.0.2.4/app/assets/javascripts/actiontext.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actiontext-7.0.2.4/app/assets/javascripts/trix.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actiontext-7.0.2.4/app/assets/stylesheets/trix.css
/Users/t/.rvm/gems/ruby-3.1.1/gems/actioncable-7.0.2.4/app/assets/javascripts/action_cable.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actioncable-7.0.2.4/app/assets/javascripts/actioncable.esm.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actioncable-7.0.2.4/app/assets/javascripts/actioncable.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/activestorage-7.0.2.4/app/assets/javascripts/activestorage.esm.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/activestorage-7.0.2.4/app/assets/javascripts/activestorage.js
/Users/t/.rvm/gems/ruby-3.1.1/gems/actionview-7.0.2.4/lib/assets/compiled/rails-ujs.js
/Users/t/git/myapp/app/javascript/application.js
/Users/t/git/myapp/app/javascript/controllers/hello_controller.js
/Users/t/git/myapp/app/javascript/controllers/index.js
/Users/t/git/myapp/app/javascript/controllers/application.js

I actually tried this with multiple other files (not committed in that dummy project ) and seem to be same exact behavior

I'm digging into this issue, playing around with source, as I want to contribute my 5-cents to Propshaft. I'll update this ticket as soon as I have something worth your time (PR maybe or whatever). Just wanted to check if this a known issue (looks like it's not)

@equivalent
Copy link
Author

equivalent commented Apr 29, 2022

I would have to write really long comment to fully explain the reason for the issue in detail. Therefore I've created screencast to better explain it -> https://youtu.be/SoT9quSjous

it's worth watching it but in short there are 2 problems:

  1. problem with browser caching old version of the file even if @import 'my-custom.css'changed (possible to fix)
  2. problem where Propshaft should tell application.css the my-custom.css has changed ( open to discussion how to fix it)

problem 1 - browser caches application.css

browser keeps in cache of given application.css content forever. This causes issue if some file imported inside it has changed. e.g. if you replace bootstrap.css with newer bootstrap.css but name is the same => browser will never detect bootstrap.css if content of application.css match one of the cached versions

this is (probably) problem when app uses Dynamic resolver. I'm not sure.

draft of a PR with proposed fix equivalent#1 (in brief: when asset digesting while using Dynamic resolver we also look at File.mtime(path) )

I'll create proper PR against rails/propshaft once I fix tests, Can you just pls look at the proposed changes if that's even worth implementing (like I've said in the video it will solve only the problem1 and not problem2)

problem 2 - application.css will not detect the imported my-custom.css has changed

if one updates value in my-custom.css there is no way to tell browser to drop cache of current application.css.

I was playing around with idea where by digesting my-custom.css I would FileUtils.touch('app/assets/application.css') and therefore thanks to File.mtime(path) in my PR that change would be detected, however I've discovered this is not worth it as browser caches the old application-xxxxxdigest.css there there is no request coming that would triggers the new version of my-custom-xxxxxdygest.css

dummy app

fresh rails app with the problem to try for yourself:

https://github.com/equivalent/propshaft-demo-app

@equivalent
Copy link
Author

more I think about it more I realize the best way how to fix this is to avoid @import("whatever.css") and rather explicitly load CSS files from within <head>:

  <head>
    <title>Myapp</title>
    <meta name="viewport" content="width=device-width,initial-scale=1">
    <%= csrf_meta_tags %>
    <%= csp_meta_tag %>

    <%= stylesheet_link_tag "bootstrap.css", "data-turbo-track": "reload" %>
    <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
    <%= stylesheet_link_tag "my-custom.css", "data-turbo-track": "reload" %>
    <%= javascript_importmap_tags %>
  </head>

The reason is that current implementation of assets.rb digests the application.css file form "content + config.version". But once developer place @import("anything.css") inside the application.css there's no backlink that it should reference to new digested version, therefore application-xxxxnewdigest.css references @import(" @import("anything-xxxxolddigest.css")`

If the gem guides developers to prefer adding multiple stylesheet_link_tags pointing to individual css files user will not have any of the problems I'm describing + gem don't need my change (although I still think it's nice to add it in just to be sure)

Quesiton is where would be the limit if developer has 50 vendor css files and 7 custom css files. Should developer add link tag for all 57 of them, or @import 50 vendor css files in application.css and hope they will never change ?

@equivalent
Copy link
Author

I had similar situation with bootstrap-icons.css which content is :

@font-face {
  font-family: "bootstrap-icons";
  src: url("/fonts/bootstrap-icons.woff2?524846017b983fc8ded9325d94ed40f3") format("woff2"),
url("/fonts/bootstrap-icons.woff?524846017b983fc8ded9325d94ed40f3") format("woff");
}

but was converted to bootstrap-icons-7cf2f17964d150413c499c4ebee2e8cafb5d9bca.css:

@font-face {
  font-family: "bootstrap-icons";
  src: url("/fonts/bootstrap-icons.woff2") format("woff2"),
url("/assets/fonts/bootstrap-icons-36952dbeb50a08a17d0dba69fa9cccde990ed10d.woff") format("woff");
}

This was because I've copied file bootstrap-icons.woff to my-app/vendor/assets/images/fonts/ but I forgot to copy the bootstrap-icons.woff2 therefore file was not found and Propshaft kept the original url string. On first load of the css file bootstrap-icons-7cf2f17964d150413c499c4ebee2e8cafb5d9bca.css browser cached this content.

Once I've copied the second file to the images/fonts/ folder I kept getting same converted css in the browser

Screenshot 2022-05-04 at 13 57 02

Now once again this was not a problem of Propshaft but of a browser (Chrome) caching the file bootstrap-icons-7cf2f17964d150413c499c4ebee2e8cafb5d9bca.css (as it content has not change), therefore if I load private window` it was working. So again this is not a bug but a feature but it causes inconvenience to developer

I think the best way how to solve it is just to recommend Developers to turn off caching while doing css changes. For example in Chrome developer can "Disable cache (while DevTools is open)"

Screenshot 2022-05-04 at 14 05 08

Screenshot 2022-05-04 at 14 03 50

note solution in my draft PR would not work -> so tracking File.mtime of a assets is useless in this case as bootstrap-icons.css did not change

@equivalent
Copy link
Author

equivalent commented May 4, 2022

...and I've just tested that by "Disable cache (while DevTools is open)" I've also solved the original issue with @import so no PR that would track File.mtime needed

But it would be nice to point this out in advance in the README.md (it's a new gem, will be hard to spot this for a newcomers like me ) I've created -> #94


To sum it all up:

This is not an issue:

/* Application styles */
@import url('/bootstrap.css');
@import url('/my-custom.css');

BUT if are in development ENV doing lot of changes your browser by default caches everything, you will notice simmilar issues. Solution => Disable cache (while DevTools is open)

ok to close this Issue from my side

@brenogazzola
Copy link
Collaborator

Sorry for the lack of feedback, things are a big rushed for me lately. I'll try to take a look next week. 🙇‍♂️

@equivalent
Copy link
Author

No worries, take your time ☕ . I've sorted it out for my usecase with the Disable cache (while DevTools is open) so I'm fine 👍

It's more to confirm if this is really the case and whether it's worth mentioning it to other developers in the Readme (e.g. #94)

@equivalent
Copy link
Author

this will solve the issue #124

@brenogazzola
Copy link
Collaborator

As I've mentioned in #124, changing headers is not enough. We need to properly calculate the digests to ensure parent digest changes when child digest changes.

Since we are increasing our focus on a "no-build" pipeline, this issue needs to be fixed before we release v1, so I'll prioritize this.

@renatonitta
Copy link

Hey @brenogazzola and @equivalent, I'm reading this issue and the PRs with the solution attempts.

I wonder if when in DEV ENV, refreshing the digest for application.css on every request could solve the issue 🤔

  1. It would work similarly to no-cache since every request would get a new file name
  2. No need to watch child files to decide when to refresh the digest or not
  3. In production, refresh the digest of application.css every time it compiles the assets

BTW when I say application.css it should consider all the files set in the load path.

What do you think?

@wlipa
Copy link
Contributor

wlipa commented Jan 4, 2024

Does the same issue exist with images that are referenced from CSS when the images are changed?

@northeastprince
Copy link

Isn't this the same problem as #123?

@pch
Copy link

pch commented Jan 28, 2024

Ran into the same issue, trying to replace css-bundling (postcss) with propshaft (nobuild).

@dhh you mentioned that the once/campfire runs vanilla css (nobuild), would you mind sharing how you set it up?

Based on this issue, I'd assume that you use multiple stylesheet_link_tag tags, rather than @import in application.css

@dhh
Copy link
Member

dhh commented Jan 28, 2024

We use stylesheet_link_tag :all in ONCE/Campfire.

@brunoprietog
Copy link

That can be a problem if you need a specific order of styles. The most representative case is to replace the default trix styles. Unless you name the file with something that is under in the alphabet than trix.css

@dhh
Copy link
Member

dhh commented Jan 28, 2024

We need to fix the underlying issue. Just saying it’s not a problem for us because we don’t use includes. But you should be able to use includes. Propshaft needs to build a transient dependency tree like we do we with template caching.

@equivalent
Copy link
Author

Just saying it’s not a problem for us because we don’t use includes ..

Same me for my projects. I've manually specified individual stylesheet_link_tag "css1", `stylesheet_link_tag "css2", .. (thx to http2 not an issue to have them many) and all ok. As long as one is aware of this issue it's easy to avoid it.

@pch

@pch
Copy link

pch commented Jan 30, 2024

@equivalent Yeah, thanks - a stylesheet_link_tag per file solves this indeed. The :all option isn't viable in my case, because order matters. A little uglier than @include in application.css, but I can live with this 😊

@wlipa
Copy link
Contributor

wlipa commented Feb 1, 2024

Directly including the CSS via stylesheet_link_tag can avoid the @import problem, but it still can leave you with stale assets such as background images. Changing the image data won't cause the CSS digest to update, so clients will hold onto the old CSS and image. If the browser dumps the image out of its cache but not the CSS, the user would potentially even get a broken link.

@dhh
Copy link
Member

dhh commented May 18, 2024

Fixed by #188

@dhh dhh closed this as completed May 18, 2024
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

Successfully merging a pull request may close this issue.

8 participants