rails / propshaft

Deliver assets for Rails
MIT License
913 stars 97 forks source link

Document how to enhance `assets:precompile` to provide compressed assets #161

Closed rmacklin closed 10 months ago

rmacklin commented 1 year ago

https://github.com/rails/propshaft/issues/158 provided some good discussion and a code example for how to provide compressed assets. This seems like useful information to add to the documentation, especially for folks who want to use Propshaft and are running their application behind NGINX with the gzip_static configuration.

nickjj commented 1 year ago

Nice one.

What do you think about changing the example to use gzip instead of Brotli since it's more supported out of the box by NGINX and other web servers? It could probably be a zero dependency "production ready" solution that uses: https://ruby-doc.org/3.2.0/exts/zlib/Zlib/GzipWriter.html

rmacklin commented 1 year ago

No strong preference from me. Would you mind using the "Add a suggestion" feature to suggest the specific changes you had in mind using GzipWriter (especially if you're able to verify the code locally - I'm on mobile at the moment)?

nickjj commented 1 year ago

Would you mind using the "Add a suggestion" feature to suggest the specific changes you had in mind using GzipWriter (especially if you're able to verify the code locally - I'm on mobile at the moment)?

No problem. I can provide confirmation on if it works or not tomorrow morning EST.

brenogazzola commented 1 year ago

Could you move this to the upgrade guide please?

Compression is a feature we believe should be handled by something other than propshaft, so this code was just a suggestion for a specific use case and therefore not something we want to put emphasis (which placing on the README does)

rmacklin commented 1 year ago

I actually considered putting this in the "Upgrading from Sprockets to Propshaft" guide, but I hesitated because even though Sprockets provided this functionality, it's still useful to people who are starting a new application with Propshaft, and those people wouldn't have any reason to look at the "Upgrading from Sprockets to Propshaft" guide.

Is there another place besides that guide and the README where this could go? Do we want to add a new document, like FAQ.md or something else? Alternatively, we could keep this in the README but replace the inline code sample with a link to this comment: https://github.com/rails/propshaft/issues/158#issuecomment-1786039189 - that would prevent this section from taking too large a percentage of the README.

nickjj commented 1 year ago

When running this code in production mode with sprockets (propshaft isn't installed), we get this error after the assets get digested:

6.710 NoMethodError: undefined method `config' for nil:NilClass
6.710 /app/config/initializers/precompile.rb:3:in `block in <main>'

Line 3 is: output_path = assembly.config.output_path, assembly is defined on line 2 with assembly = Rails.application.assets.

Steps to reproduce in a repeatable project:

# I've annotated some of the code changes with comments.
# These aren't meant to add to the docs. It's context for why I changed them.

Rake::Task["assets:precompile"].enhance do
  assembly    = Rails.application.assets
  output_path = assembly.config.output_path

  assembly.load_path.assets.each do |asset|
    asset_path      = output_path.join(asset.digested_path)
    compressed_path = output_path.join(asset.digested_path.to_s + ".gz")

    unless compressed_path.exist?
      # I changed this from Propshaft to Rails.
      Rails.logger.info "Compressing #{asset.digested_path}"

      # Zlib::BEST_COMPRESSION uses 9 as the compression. The default is
      # Zlib::DEFAULT_COMPRESSION which is 6. More details can be found at:
      #     https://docs.ruby-lang.org/en/3.0/Zlib/Deflate.html
      Zlib::GzipWriter.open(compressed_path, Zlib::BEST_COMPRESSION) do |gz|
        gz.mtime     = File.mtime(asset_path)
        gz.orig_name = asset_path
        gz.write IO.binread(asset_path)
      end
    end
  end
end if Rake::Task.task_defined?("assets:precompile")
export COMPOSE_PROFILES=postgres,redis,web,worker,cable
export RAILS_ENV=production
export NODE_ENV=production
export RAILS_SERVE_STATIC_FILES=true

The error will happen during the build process.

brenogazzola commented 1 year ago

When running this code in production mode with sprockets (propshaft isn't installed)

@nickjj This code is specific to Propshaft, which does not have a compression feature and should not be used in a project using Sprockets, which has that feature.

brenogazzola commented 1 year ago

@rmacklin How about we take take a page out of Sprockets book and create a "guides" folder? Then we can move the upgrade guide to there too and create an "extending_propshaft.md" file and have this code and explanation there. Could also move the notice about the file watcher that is the readme me to there. And finally edit the readme to point to those guides.

nickjj commented 1 year ago

This code is specific to Propshaft, which does not have a compression feature and should not be used in a project using Sprockets, which has that feature.

Right but the code is failing on assembly = Rails.application.assets because it's coming back as nil. Is Rails.application.assets something Propshaft adds to Rails?

brenogazzola commented 1 year ago

If you are asking this just as a curiosity, then .assets variable is something that both Sprockets and Propshaft assign a value to during application boot. My best guess as to why you are getting a null pointer with Sprockets but it works with Propshaft, is that Sprockets sets it later than Propshaft.

You could probably use Rails to_prepare or after_initialize to fix the null pointer, but then you'd just get another error, since the object Sprockets assigns to assets does not have a load_path method.

nickjj commented 1 year ago

I see. I don't want to clutter this PR with unrelated comments but Propshaft's upgrade guide mentions deleting the assets.rb initializer but my initializer had Rails.application.config.assets.paths << "/node_modules" and the Propshaft docs don't mention how to add new load paths.

I'll need to make that customization with Propshaft to be able to test this PR.

brenogazzola commented 1 year ago

If you are using node_modules, you are probably using a bundler like webpack/esbuild.

In this cases there's not need to add node_modules to the path. Instead, simply have your bundler place the compiled assets (css/js) into app/assets/builds, and propshaft will pick them up automatically.

It's pretty much what the bundling gems (jsbundling-rails and cssbundling-rails) do.

nickjj commented 1 year ago

I'm using esbuild and it's configured to place assets into /node_modules. It's important that the path doesn't change because it's a non-volume mounted location of the file system within Docker.

How can I configure Propshaft to include /node_modules as an assets path similar to sprockets?

brenogazzola commented 1 year ago

Don’t do that. Propshaft paths do not work like Sprockets paths.

Anything you add to paths will be copied to the public/assets folder by Propshaft. I doubt you want the entirety of the node_modules folder there, as that would be thousands of files and hundreds of megabytes, and it would massively slow down the precompile.

Like I said, have your bundler, which uses node_modules folder as the source of the libs for your own js files, place the files it generates in app/assets/builds and from there propshaft will copy them to public/assets. That’s the correct workflow to integrate Propshaft with bundlers like webpack/esbuild/etc.

brenogazzola commented 1 year ago

That said, if you really want to do that to see what happens, instead of removing the assets.rb file, place this line in it:

Rails.application.config.assets.paths.unshift Rails.root.join("node_modules").to_s
nickjj commented 1 year ago

Just to make sure I'm 100% following here. Here's what I do today with a combo of jsbundling, cssbundling and sprockets. Everything gets bundled, digested and gzipped as expected and my bundles only include my app's custom JS and CSS:

With Propshaft it sounds like step 1 doesn't change. Step 2 can't change because that's where I Yarn install everything to. The part I'm not clear on is step 3. The upgrade guide says to delete the initializer where this step was previously included but it has no alternative. Maybe this path configuration isn't needed at all with Propshaft and this whole concept goes away? In other words I'm good to go out of the box with my old steps 1 and 2 with Propshaft?

brenogazzola commented 1 year ago

Correct, you don’t need step 3. Propshaft does not care about node modules. It’s esbuild responsibility to read stuff from there when compiling your files. Propshaft only cares about where your compiled files will be places and that’s the builds folder.

nickjj commented 1 year ago

Everything mostly worked but when it's set as an initializer I get an error at runtime (more details soon).

I also had to make 1 adjustment to the above code snippet. I had to change gz.orig_name = asset_path to gz.orig_name = asset_path.to_s. Without that it got an error of TypeError: no implicit conversion of Pathname into String.

First the good news. When I added all of the Propshaft changes from the upgrade guide I built my Docker image which performs a precompile during the build process when Rails env is set to production.

It produced this output on disk:

-rw-r--r-- 1 ruby ruby   1808 Nov  3 01:30 .manifest.json
-rw-r--r-- 1 ruby ruby  16529 Nov  3 01:30 action_cable-047c3571788ef5bf53d3c094ea88f8d02282fd55.js
-rw-r--r-- 1 ruby ruby   3998 Nov  3 01:30 action_cable-047c3571788ef5bf53d3c094ea88f8d02282fd55.js.gz
-rw-r--r-- 1 ruby ruby  16399 Nov  3 01:30 actioncable-20c99abd909914d08a5152d354090219f1e77f7d.js
-rw-r--r-- 1 ruby ruby   3909 Nov  3 01:30 actioncable-20c99abd909914d08a5152d354090219f1e77f7d.js.gz
-rw-r--r-- 1 ruby ruby  14738 Nov  3 01:30 actioncable.esm-8979e41319eac55e88fa3d4c85d4edd0b4f6b57b.js
-rw-r--r-- 1 ruby ruby   3670 Nov  3 01:30 actioncable.esm-8979e41319eac55e88fa3d4c85d4edd0b4f6b57b.js.gz
-rw-r--r-- 1 ruby ruby  28309 Nov  3 01:30 actiontext-0447cab0a183f49e8f41c42543f91f30956cdbaa.js
-rw-r--r-- 1 ruby ruby   6465 Nov  3 01:30 actiontext-0447cab0a183f49e8f41c42543f91f30956cdbaa.js.gz
-rw-r--r-- 1 ruby ruby  29407 Nov  3 01:30 activestorage-48abb62e59b504340905a0a96e3bcb565469bfa3.js
-rw-r--r-- 1 ruby ruby   6461 Nov  3 01:30 activestorage-48abb62e59b504340905a0a96e3bcb565469bfa3.js.gz
-rw-r--r-- 1 ruby ruby  27275 Nov  3 01:30 activestorage.esm-4a171e28ffc0c56797fe2f19c02def0643629ea1.js
-rw-r--r-- 1 ruby ruby   6241 Nov  3 01:30 activestorage.esm-4a171e28ffc0c56797fe2f19c02def0643629ea1.js.gz
-rw-r--r-- 1 ruby ruby 132307 Nov  3 01:30 application-15bcd68f4ffb825ac7ce8e396e922982f3a53cbb.js
-rw-r--r-- 1 ruby ruby  33447 Nov  3 01:30 application-15bcd68f4ffb825ac7ce8e396e922982f3a53cbb.js.gz
-rw-r--r-- 1 ruby ruby   5790 Nov  3 01:30 application-9b81a9bc47231186662cba4ca0a8c14394e60da1.css
-rw-r--r-- 1 ruby ruby   2178 Nov  3 01:30 application-9b81a9bc47231186662cba4ca0a8c14394e60da1.css.gz
-rw-r--r-- 1 ruby ruby    233 Nov  3 01:30 application.tailwind-1c23e218248539ba0d65cd914e9cbf62b47b2e76.css
-rw-r--r-- 1 ruby ruby    246 Nov  3 01:30 application.tailwind-1c23e218248539ba0d65cd914e9cbf62b47b2e76.css.gz
-rw-r--r-- 1 ruby ruby  24018 Nov  3 01:30 rails-ujs-3de06f48a4b71b701d5652e806d90b95fe91fde0.js
-rw-r--r-- 1 ruby ruby   5547 Nov  3 01:30 rails-ujs-3de06f48a4b71b701d5652e806d90b95fe91fde0.js.gz
-rw-r--r-- 1 ruby ruby  22529 Nov  3 01:30 rails-ujs.esm-f4416132827118d78ba56fdd1d02d5b4f0ed4612.js
-rw-r--r-- 1 ruby ruby   5411 Nov  3 01:30 rails-ujs.esm-f4416132827118d78ba56fdd1d02d5b4f0ed4612.js.gz
-rw-r--r-- 1 ruby ruby  18358 Nov  3 01:30 ruby-on-rails-64ba983ed6afc0548b4a28b6e8375770af3cc47f.png
-rw-r--r-- 1 ruby ruby  18162 Nov  3 01:30 ruby-on-rails-64ba983ed6afc0548b4a28b6e8375770af3cc47f.png.gz
-rw-r--r-- 1 ruby ruby  88790 Nov  3 01:30 stimulus-686702a0e4888429bc44efe99c0e83a8885d7fad.js
-rw-r--r-- 1 ruby ruby  15289 Nov  3 01:30 stimulus-686702a0e4888429bc44efe99c0e83a8885d7fad.js.gz
-rw-r--r-- 1 ruby ruby   1747 Nov  3 01:30 stimulus-autoloader-045d8bc2c928fe8f6696d419c558f2b94571ab91.js
-rw-r--r-- 1 ruby ruby    763 Nov  3 01:30 stimulus-autoloader-045d8bc2c928fe8f6696d419c558f2b94571ab91.js.gz
-rw-r--r-- 1 ruby ruby    989 Nov  3 01:30 stimulus-importmap-autoloader-482dc40cf850bd23525e97b144d2eb4ceaf58eea.js
-rw-r--r-- 1 ruby ruby    603 Nov  3 01:30 stimulus-importmap-autoloader-482dc40cf850bd23525e97b144d2eb4ceaf58eea.js.gz
-rw-r--r-- 1 ruby ruby   3315 Nov  3 01:30 stimulus-loading-25917588565633495ac04a032df7c72f2a9368de.js
-rw-r--r-- 1 ruby ruby   1103 Nov  3 01:30 stimulus-loading-25917588565633495ac04a032df7c72f2a9368de.js.gz
-rw-r--r-- 1 ruby ruby  45689 Nov  3 01:30 stimulus.min-7ea3d58b7f4507e3603ec999251ff60d16431a30.js
-rw-r--r-- 1 ruby ruby  11150 Nov  3 01:30 stimulus.min-7ea3d58b7f4507e3603ec999251ff60d16431a30.js.gz
-rw-r--r-- 1 ruby ruby 164428 Nov  3 01:30 stimulus.min.js-e528a1dec846262ee5bed747878e9332209d754e.map
-rw-r--r-- 1 ruby ruby  39025 Nov  3 01:30 stimulus.min.js-e528a1dec846262ee5bed747878e9332209d754e.map.gz
-rw-r--r-- 1 ruby ruby  19972 Nov  3 01:30 trix-8c12b8d7b376f8a77beda6a48023f3f5ae8f44ad.css
-rw-r--r-- 1 ruby ruby   4330 Nov  3 01:30 trix-8c12b8d7b376f8a77beda6a48023f3f5ae8f44ad.css.gz
-rw-r--r-- 1 ruby ruby 382297 Nov  3 01:30 trix-c658be6d5ffb915e28499a7b89a8f7beae3f9dc9.js
-rw-r--r-- 1 ruby ruby  73156 Nov  3 01:30 trix-c658be6d5ffb915e28499a7b89a8f7beae3f9dc9.js.gz
-rw-r--r-- 1 ruby ruby 144979 Nov  3 01:30 turbo-74f24215f541935536b939f926c926debbb86150.js
-rw-r--r-- 1 ruby ruby  29867 Nov  3 01:30 turbo-74f24215f541935536b939f926c926debbb86150.js.gz
-rw-r--r-- 1 ruby ruby  87620 Nov  3 01:30 turbo.min-b405a8972a8538cd316a47ac0fa9b7de2439c484.js
-rw-r--r-- 1 ruby ruby  22619 Nov  3 01:30 turbo.min-b405a8972a8538cd316a47ac0fa9b7de2439c484.js.gz
-rw-r--r-- 1 ruby ruby 266758 Nov  3 01:30 turbo.min.js-880bd80cfd9fd13e6a22c837fab55b40f7b64beb.map
-rw-r--r-- 1 ruby ruby  68739 Nov  3 01:30 turbo.min.js-880bd80cfd9fd13e6a22c837fab55b40f7b64beb.map.gz

The first few bytes of the gzipped files are 1f 8b which suggests they really are gzipped files.

$ od --format=x1 --read-bytes=10 turbo.min-b405a8972a8538cd316a47ac0fa9b7de2439c484.js.gz
0000000 1f 8b 08 08 4a 4d 44 65 02 03

I was also able to successfully gunzip the file and confirm it produces a plain text ascii file with the correct contents.

However at runtime when the container starts I get this error:

hellorails-web-1       | /app/config/initializers/precompile.rb:26:in `<main>': uninitialized constant Rake::Task (NameError)
hellorails-web-1       |
hellorails-web-1       | end if Rake::Task.task_defined?("assets:precompile")
hellorails-web-1       |            ^^^^^^
hellorails-web-1       |        from /usr/local/bundle/gems/railties-7.1.1/lib/rails/engine.rb:683:in `load'

If I remove the if condition then I get the same error but on Rake::Task["assets:precompile"].enhance. I tried adding require "rake/task" but then started to get other errors such as undefined method 'application' for Rake:Module.

Technically assets shouldn't be precompiled at runtime but I guess Rails is maybe trying to load this code even if it's not executing it? In which case, any suggestions on how to make this startup properly?

brenogazzola commented 10 months ago

@nickjj Need to change the if in that code I gave you:

Rake::Task["assets:precompile"].enhance do
  # Code here
end if defined?(Rack) && Rake::Task.task_defined?("assets:precompile")
nickjj commented 10 months ago

@brenogazzola It fails with the same type of error as before:

hellorails-web-1       | /app/config/initializers/compress_assets.rb:19:in `<main>': uninitialized constant Rake::Task (NameError)
hellorails-web-1       |
hellorails-web-1       | end if defined?(Rack) && Rake::Task.task_defined?("assets:precompile")
dhh commented 10 months ago

We are working on a new solution to provide compression out of the box without CDN. See the Airlock issue on the Rails 8 milestone. I think that's a better path.

nickjj commented 10 months ago

Would it still be possible to provide a working initializer here in case Airlock doesn't solve this use case?

Based on the milestone it sounds like it would compress assets on the fly similar to how nginx would compress them, but the original issue for this use case was about pre-compressing them because it's more efficient. The original issue is here btw: https://github.com/rails/propshaft/issues/158

dhh commented 10 months ago

I don’t think that’ll be a concern of high enough importance for most to bother with. Airlock will only compress once per asset, though it will do so on demand. But computers are fast enough that it isn’t a concern I think most people should have to even think about.

I want to keep Propshaft as lean as possible. That includes which seeds to plant in people’s minds about what’s needed or not when reading the docs.

nickjj commented 10 months ago

Ok thanks, I'll drop a request in the Rails forums to see if we can figure out debugging this almost-working-initializer for folks who plan to stick with using nginx which doesn't only compress assets once.

morgoth commented 9 months ago

I understand to not include it in the readme, but maybe it would be helpful to put it in the upgrade guide in section about migrating from sprockets. @dhh what do you think?

dhh commented 9 months ago

I think it'll all be moot because Thruster will be the default in Rails 8, and that'll provide the compression needed.