Add an option to only pick assets from app/assets/stylesheets using stylesheet_link_tag :app

[brunoprietog]

This is a simple proof of concept to resolve the issue raised in this comment.

The idea is that when using the helper it only includes the CSS files contained in the app/assets/stylesheets folder. This way, no external CSS styles will be included in the application, for example from engines like Mission Control.

Obviously, this could break applications that are using this and assume that they include all CSS files from all engines automatically, such as Trix. They would have to be included manually.

I see this as an intermediate option between including all CSS files from the load path or not including any and using @import manually for each file.

[dhh]

I think there's something here. But I'm thinking maybe we could let :all remain everything, thus keeping backwards compatibility, but then add :application to do what you're doing here. And then make :application the new default in new apps?

[brunoprietog]

Yes, makes sense! But maybe another name? I can't think of it at the moment, but I feel like the difference between :application and "application" is very fuzzy.

[dhh]

Could do :app to match app/

[brunoprietog]

Sounds good. I've updated the code to cover those cases.

Let me know if there's another better way to implement it.

[dhh]

Don't think this is the right level to have something like this. Let's keep that in the helper.

[brunoprietog]

Right, I've moved it to the helper. Do you think it would be better to memoize it?

[dhh]

Think we can do this with Rails.application.assets.load_path.dup.tap { |l| l.paths = [ File.join(Rails.root, "app", "assets", "stylesheets") ] } or something like that. So you don't have to repeat the configuration.

[dhh]

Actually I see there's not a setter on paths. We can just add that, but it'll need to clear the cache when set.

[brunoprietog]

I did it, although I had to add a setter to clear the cache.

[brunoprietog]

Actually I see there's not a setter on paths. We can just add that, but it'll need to clear the cache when set.

Ah yes, I just read this

[dhh]

You should be able to just use Rails.root.join("app/assets/stylesheets") here.

[brunoprietog]

Done

[dhh]

Actually, on second thought, we don't need the load_path or the helper method to do this. We can just do:

def app_stylesheet_paths
  Rails.root.join("app/assets/stylesheets").then do |dir|
    dir.glob("**/*.css").collect { |f| f.relative_path_from(dir).to_s }.sort
  end
end

Although trying to think you might/probably want to cache this in production.

[dhh]

Yeah, the cache question is an issue. Same for :all. We shouldn't be doing file stats on every request. Think we need a way to get this into a proper object with caching that's reset by a sweeper, like LoadPath.

[dhh]

Actually maybe caching doesn't matter. Just did a simple test on HEY:

haystack(dev)* Benchmark.measure { 1000.times { Rails.root.join("app/assets/stylesheets").then do |dir|
haystack(dev)*     dir.glob("**/*.css").collect { |f| f.relative_path_from(dir).to_s }.sort
haystack(dev)*     end
haystack(dev)> } }.real / 1000
=> 0.00025197141902754083

So that's an overhead of 0.2ms. Which probably means that the file system or whatever is already providing the cache.

Guess it could be worth looking at a more pathological case, maybe? Slow CPU + HDD might well give a different result.

[dhh]

That benchmark wasn't correct. This is right:

Benchmark.measure { 1000.times { Rails.root.join("app/assets/stylesheets").then { |dir| dir.glob("**/*.scss").collect { |f| f.relative_path_from(dir).to_s } } } }.real / 1000

Which sets the price at 1ms for 263 files on a very fast CPU + SSD. That's not great. So back to looking at caching that's stable in production!

[dhh]

Which actually gives me wonder whether we should have a generalized cache like this. Stable in production, pass-through in dev/test.

[dhh]

Ended up pursuing the cached path in #190.

[brunoprietog]

Great! Sorry for not following up on this yesterday. I had to go out