From e7450cc6da49fbc4cf6f251d7d9297aaf457e240 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 30 Apr 2025 08:59:32 +0100 Subject: [PATCH] DEV: Migrate from sprockets to propshaft for assets (#32475) We are no longer using any of the transpilation/bundling features of Sprockets. We only use it to serve assets in development, and then collect & fingerprint them in production. This commit switches us to use the more modern "Propshaft" gem for that functionality. Propshaft is much simpler than Sprockets. Instead of taking a combination of paths + "precompile" list, Propshaft simply assumes all files in the configured directory are required in production. Previously we had some base paths configured quite high in the directory structure, and then only precompiled selected assets within the directory. That's no longer possible, so this commit refactors those places (mostly plugin-related) to use dedicated directories under `app/assets/generated/`. Another difference is that Propshaft applies asset digests in development as well as production. This is great for caching & dev/prod consistency, but does mean some small changes were required in tests. We previously had some freedom-patches applied to Sprockets. Some of those had to be ported across to Propshaft. We now have three patches: 1. Skip adding digest hashes to webpack-generated chunks (which are already digested, and referred to from other js files) 2. Avoid raising errors for missing assets in test mode. We don't always compile assets before running basic RSpec tests. 3. Maintain relative paths for sourcemap URLs, so that files don't need to be recompiled depending on their CDN path Significant refactors are made to the `assets.rake` and `s3.rake` tasks, which rely on implementation details of Sprockets/Propshaft. --- .gitignore | 1 + Gemfile | 7 +- Gemfile.lock | 19 ++- config/application.rb | 29 ----- config/environments/development.rb | 2 - config/initializers/assets.rb | 43 +++---- config/routes.rb | 8 -- lib/discourse.rb | 13 ++- lib/discourse_js_processor.rb | 38 ------ lib/discourse_sourcemapping_url_processor.rb | 16 --- lib/freedom_patches/propshaft_patches.rb | 35 ++++++ lib/freedom_patches/sprockets_patches.rb | 85 -------------- lib/middleware/turbo_dev.rb | 36 ------ lib/plugin/instance.rb | 38 +++--- lib/tasks/assets.rake | 110 ++++-------------- lib/tasks/s3.rake | 33 ++---- spec/helpers/application_helper_spec.rb | 46 ++------ spec/lib/discourse_js_processor_spec.rb | 17 --- ...course_sourcemapping_url_processor_spec.rb | 33 ------ .../services/push_notification_pusher_spec.rb | 8 +- 20 files changed, 139 insertions(+), 478 deletions(-) delete mode 100644 lib/discourse_sourcemapping_url_processor.rb create mode 100644 lib/freedom_patches/propshaft_patches.rb delete mode 100644 lib/freedom_patches/sprockets_patches.rb delete mode 100644 lib/middleware/turbo_dev.rb delete mode 100644 spec/lib/discourse_sourcemapping_url_processor_spec.rb diff --git a/.gitignore b/.gitignore index ff0533248a6..8a1ff6d189e 100644 --- a/.gitignore +++ b/.gitignore @@ -79,6 +79,7 @@ yarn-error.log # Auto-generated plugin JS assets /app/assets/javascripts/plugins/* +/app/assets/generated # Generated API documentation files openapi/* diff --git a/Gemfile b/Gemfile index 64d85265867..f86863eecf7 100644 --- a/Gemfile +++ b/Gemfile @@ -15,14 +15,11 @@ gem "activemodel", "~> 7.2.0" gem "activerecord", "~> 7.2.0" gem "activesupport", "~> 7.2.0" gem "railties", "~> 7.2.0" -gem "sprockets-rails" + +gem "propshaft" gem "json" -# TODO: At the moment Discourse does not work with Sprockets 4, we would need to correct internals -# We intend to drop sprockets rather than upgrade to 4.x -gem "sprockets", "~> 3.7.3" - # this will eventually be added to rails, # allows us to precompile all our templates in the unicorn master gem "actionview_precompiler", require: false diff --git a/Gemfile.lock b/Gemfile.lock index 8229ee61dba..17e55c41657 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -361,6 +361,11 @@ GEM prettyprint (0.2.0) prism (1.4.0) progress (3.6.0) + propshaft (1.1.0) + actionpack (>= 7.0.0) + activesupport (>= 7.0.0) + rack + railties (>= 7.0.0) pry (0.15.2) coderay (~> 1.1) method_source (~> 1.0) @@ -581,14 +586,6 @@ GEM snaky_hash (2.0.1) hashie version_gem (~> 1.1, >= 1.1.1) - sprockets (3.7.5) - base64 - concurrent-ruby (~> 1.0) - rack (> 1, < 3) - sprockets-rails (3.5.2) - actionpack (>= 6.1) - activesupport (>= 6.1) - sprockets (>= 3.0.0) sqlite3 (2.6.0-aarch64-linux-gnu) sqlite3 (2.6.0-aarch64-linux-musl) sqlite3 (2.6.0-arm-linux-gnu) @@ -738,6 +735,7 @@ DEPENDENCIES parallel parallel_tests pg + propshaft pry-byebug pry-rails pry-stack_explorer @@ -779,8 +777,6 @@ DEPENDENCIES shoulda-matchers sidekiq simplecov - sprockets (~> 3.7.3) - sprockets-rails sqlite3 sshkey stackprof @@ -977,6 +973,7 @@ CHECKSUMS prettyprint (0.2.0) sha256=2bc9e15581a94742064a3cc8b0fb9d45aae3d03a1baa6ef80922627a0766f193 prism (1.4.0) sha256=dc0e3e00e93160213dc2a65519d9002a4a1e7b962db57d444cf1a71565bb703e progress (3.6.0) sha256=360ed306dfa43d6174e847d563c70736dca249e2333cfec4b0387306c86cd573 + propshaft (1.1.0) sha256=d389361faf66aeb17e8d204828962c1e506edd14a1a17adb3fa475435c070f6b pry (0.15.2) sha256=12d54b8640d3fa29c9211dd4ffb08f3fd8bf7a4fd9b5a73ce5b59c8709385b6b pry-byebug (3.11.0) sha256=0b0abb7d309bc7f00044d512a3c8567274f7012b944b38becc8440439a1cea72 pry-rails (0.3.11) sha256=a69e28e24a34d75d1f60bcf241192a54253f8f7ef8a62cba1e75750a9653593d @@ -1062,8 +1059,6 @@ CHECKSUMS simplecov_json_formatter (0.1.4) sha256=529418fbe8de1713ac2b2d612aa3daa56d316975d307244399fa4838c601b428 simpleidn (0.2.3) sha256=08ce96f03fa1605286be22651ba0fc9c0b2d6272c9b27a260bc88be05b0d2c29 snaky_hash (2.0.1) sha256=1ac87ec157fcfe7a460e821e0cd48ae1e6f5e3e082ab520f03f31a9259dbdc31 - sprockets (3.7.5) sha256=72c20f256548f8a37fe7db41d96be86c3262fddaf4ebe9d69ec8317394fed383 - sprockets-rails (3.5.2) sha256=a9e88e6ce9f8c912d349aa5401509165ec42326baf9e942a85de4b76dbc4119e sqlite3 (2.6.0-aarch64-linux-gnu) sha256=febc29bd7037695779d6b482fac7f7add9af7b420a1c5120ccff79213415975e sqlite3 (2.6.0-aarch64-linux-musl) sha256=d235cf89ba96067462562bc71adf5309363a9fee09a1864667f45799f26825ca sqlite3 (2.6.0-arm-linux-gnu) sha256=5f7e1160ad6bf6bfc0d42d1a7de95554b00a5dc7375953070d9633fedadc93bd diff --git a/config/application.rb b/config/application.rb index 11212513864..7a8fe47a3ba 100644 --- a/config/application.rb +++ b/config/application.rb @@ -5,7 +5,6 @@ require "active_record/railtie" require "action_controller/railtie" require "action_view/railtie" require "action_mailer/railtie" -require "sprockets/railtie" # Plugin related stuff require_relative "../lib/plugin" @@ -168,34 +167,6 @@ module Discourse require "middleware/discourse_public_exceptions" config.exceptions_app = Middleware::DiscoursePublicExceptions.new(Rails.public_path) - require "discourse_js_processor" - require "discourse_sourcemapping_url_processor" - - Sprockets.register_mime_type "application/javascript", - extensions: %w[.js .es6 .js.es6], - charset: :unicode - Sprockets.register_postprocessor "application/javascript", DiscourseJsProcessor - - class SprocketsSassUnsupported - def self.call(*args) - raise "Discourse does not support compiling scss/sass files via Sprockets" - end - end - - Sprockets.register_engine(".sass", SprocketsSassUnsupported, silence_deprecation: true) - Sprockets.register_engine(".scss", SprocketsSassUnsupported, silence_deprecation: true) - - Discourse::Application.initializer :prepend_ember_assets do |app| - # Needs to be in its own initializer so it runs after the append_assets_path initializer defined by Sprockets - app - .config - .assets - .paths.unshift "#{app.config.root}/app/assets/javascripts/discourse/dist/assets" - Sprockets.unregister_postprocessor "application/javascript", - Sprockets::Rails::SourcemappingUrlProcessor - Sprockets.register_postprocessor "application/javascript", DiscourseSourcemappingUrlProcessor - end - require "discourse_redis" require "logster/redis_store" # Use redis for our cache diff --git a/config/environments/development.rb b/config/environments/development.rb index 4dab096a286..27c86d41e42 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -64,8 +64,6 @@ Discourse::Application.configure do config.hosts.concat(hosts.split(",")) end - require "middleware/turbo_dev" - config.middleware.insert 0, Middleware::TurboDev require "middleware/missing_avatars" config.middleware.insert 1, Middleware::MissingAvatars diff --git a/config/initializers/assets.rb b/config/initializers/assets.rb index 578cf8d2c5e..1c7ccdcfa99 100644 --- a/config/initializers/assets.rb +++ b/config/initializers/assets.rb @@ -9,34 +9,23 @@ Rails.application.config.assets.enabled = true Rails.application.config.assets.version = "2-#{GlobalSetting.asset_url_salt}" # Add additional assets to the asset load path. -Rails.application.config.assets.paths << "#{Rails.root}/public/javascripts" +Rails.application.config.assets.paths.push( + "#{Rails.root}/public/javascripts", + "#{Rails.root}/app/assets/javascripts/discourse/dist/assets", +) -# Precompile additional assets. -# application.js, application.css, and all non-JS/CSS in the app/assets -# folder are already added. +Rails.application.config.assets.paths.push( + *Discourse.plugins.map { |p| "#{Rails.root}/app/assets/generated/#{p.directory_name}" }, +) -# explicitly precompile any images in plugins ( /assets/images ) path -Rails.application.config.assets.precompile += [ - lambda do |filename, path| - path =~ %r{assets/images} && !%w[.js .css].include?(File.extname(filename)) - end, -] +# These paths are added automatically by propshaft, but we don't want them +Rails.application.config.assets.excluded_paths.push( + "#{Rails.root}/app/assets/generated", + "#{Rails.root}/app/assets/javascripts", + "#{Rails.root}/app/assets/stylesheets", +) -Rails.application.config.assets.precompile += %w[scripts/discourse-test-listen-boot] - -Rails.application.config.assets.precompile << lambda do |logical_path, filename| - filename.start_with?(EmberCli.dist_dir) && EmberCli.assets.include?(logical_path) +# We don't need/want most of Propshaft's preprocessing. Only keep the JS sourcemap handler +Rails.application.config.assets.compilers.filter! do |type, compiler| + type == "text/javascript" && compiler == Propshaft::Compiler::SourceMappingUrls end - -# out of the box sprockets 3 grabs loose files that are hanging in assets, -# the exclusion list does not include hbs so you double compile all this stuff -Rails.application.config.assets.precompile.delete(Sprockets::Railtie::LOOSE_APP_ASSETS) - -# We don't want application from node_modules, only from the root -Rails.application.config.assets.precompile.delete(%r{(?:/|\\|\A)application\.(css|js)$}) - -Discourse - .find_plugin_js_assets(include_disabled: true) - .each do |file| - Rails.application.config.assets.precompile << "#{file}.js" if file.end_with?("_extra") - end diff --git a/config/routes.rb b/config/routes.rb index 94d7f6fd115..101b2fc63bc 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1579,14 +1579,6 @@ Discourse::Application.routes.draw do resources :drafts, only: %i[index create show destroy] get "/service-worker.js" => "static#service_worker_asset", :format => :js - if service_worker_asset = Rails.application.assets_manifest.assets["service-worker.js"] - # https://developers.google.com/web/fundamentals/codelabs/debugging-service-workers/ - # Normally the browser will wait until a user closes all tabs that contain the - # current site before updating to a new Service Worker. - # Support the old Service Worker path to avoid routing error filling up the - # logs. - get service_worker_asset => "static#service_worker_asset", :format => :js - end get "cdn_asset/:site/*path" => "static#cdn_asset", :format => false, diff --git a/lib/discourse.rb b/lib/discourse.rb index 66b657d1a9b..a95dab852d6 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -466,7 +466,18 @@ module Discourse def self.assets_digest @assets_digest ||= begin - digest = Digest::MD5.hexdigest(ActionView::Base.assets_manifest.assets.values.sort.join) + digest = + Digest::MD5.hexdigest( + Rails + .application + .assets + .load_path + .assets + .map(&:digested_path) + .map(&:to_s) + .sort + .join("|"), + ) channel = "/global/asset-version" message = MessageBus.last_message(channel) diff --git a/lib/discourse_js_processor.rb b/lib/discourse_js_processor.rb index a234a18fb3c..fdfa68626fb 100644 --- a/lib/discourse_js_processor.rb +++ b/lib/discourse_js_processor.rb @@ -6,49 +6,11 @@ class DiscourseJsProcessor class TranspileError < StandardError end - def self.ember_cli?(filename) - filename.include?("/app/assets/javascripts/discourse/dist/") - end - - def self.call(input) - root_path = input[:load_path] || "" - logical_path = - (input[:filename] || "").sub(root_path, "").gsub(/\.(js|es6).*$/, "").sub(%r{^/}, "") - data = input[:data] - - data = transpile(data, root_path, logical_path) if should_transpile?(input[:filename]) - - { data: data } - end - def self.transpile(data, root_path, logical_path, theme_id: nil, extension: nil) transpiler = Transpiler.new(skip_module: skip_module?(data)) transpiler.perform(data, root_path, logical_path, theme_id: theme_id, extension: extension) end - def self.should_transpile?(filename) - filename ||= "" - - # skip ember cli - return false if ember_cli?(filename) - - # es6 is always transpiled - return true if filename.end_with?(".es6") || filename.end_with?(".es6.erb") - - # For .js check the path... - return false unless filename.end_with?(".js") || filename.end_with?(".js.erb") - - relative_path = filename.sub(Rails.root.to_s, "").sub(%r{^/*}, "") - - js_root = "app/assets/javascripts" - test_root = "test/javascripts" - - return false if relative_path.start_with?("#{js_root}/locales/") - return false if relative_path.start_with?("#{js_root}/plugins/") - - !!(relative_path =~ %r{^#{js_root}/[^/]+/} || relative_path =~ %r{^#{test_root}/[^/]+/}) - end - def self.skip_module?(data) !!(data.present? && data =~ %r{^// discourse-skip-module$}) end diff --git a/lib/discourse_sourcemapping_url_processor.rb b/lib/discourse_sourcemapping_url_processor.rb deleted file mode 100644 index 29095863c50..00000000000 --- a/lib/discourse_sourcemapping_url_processor.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -# This postprocessor rewrites `//sourceMappingURL=` comments to include the hashed filename of the map. -# As a side effect, the default implementation also replaces relative sourcemap URLs with absolute URLs, including the CDN domain. -# We want to preserve the relative nature of the URLs, so that compiled JS is portable across sites with differing CDN configurations. -class DiscourseSourcemappingUrlProcessor < Sprockets::Rails::SourcemappingUrlProcessor - def self.sourcemap_asset_path(sourcemap_logical_path, context:) - result = super(sourcemap_logical_path, context: context) - if (File.basename(sourcemap_logical_path) === sourcemap_logical_path) || - sourcemap_logical_path.start_with?("plugins/") - # If the original sourcemap reference is relative, keep it relative - result = File.basename(result) - end - result - end -end diff --git a/lib/freedom_patches/propshaft_patches.rb b/lib/freedom_patches/propshaft_patches.rb new file mode 100644 index 00000000000..218b2660743 --- /dev/null +++ b/lib/freedom_patches/propshaft_patches.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +Propshaft::Asset.prepend( + Module.new do + def already_digested? + logical_path.to_s.start_with?("chunk.") || super + end + end, +) + +Propshaft::Helper.prepend( + Module.new do + def compute_asset_path(path, options = {}) + super + rescue Propshaft::MissingAssetError => e + if Rails.env.test? + # Assets might not be compiled in test mode. Just return a fake path + "/assets/#{path.sub(".", "-aaaaaaaa.")}" + else + raise e + end + end + end, +) + +Propshaft::Compiler::SourceMappingUrls.prepend( + Module.new do + def source_mapping_url(*args) + # Propshaft insists on converting sourcemap URLs to absolute paths. We want to keep + # relative paths so that we can serve assets from different subdirectories without needing + # to recompile them + super.gsub(%r{sourceMappingURL=\S+/([^/]+\.map)}, 'sourceMappingURL=\1') + end + end, +) diff --git a/lib/freedom_patches/sprockets_patches.rb b/lib/freedom_patches/sprockets_patches.rb deleted file mode 100644 index 5ab3035e0a3..00000000000 --- a/lib/freedom_patches/sprockets_patches.rb +++ /dev/null @@ -1,85 +0,0 @@ -# frozen_string_literal: true - -# This contains two patches to make sprockets more tolerable in dev -# -# 1. Stop computing asset paths which triggers sprockets to do mountains of work -# All our assets in dev are in the /assets folder anyway -# -# 2. Stop using a concatenator that does tons of work checking for semicolons when -# when rebuilding an asset - -module FreedomPatches - module SprocketsPatches - def self.concat_javascript_sources(buf, source) - if buf.bytesize > 0 - # CODE REMOVED HERE - buf << ";" # unless string_end_with_semicolon?(buf) - buf << "\n" # unless buf.end_with?("\n") - end - buf << source - end - - if Rails.env.local? - Sprockets.register_bundle_metadata_reducer "application/javascript", - :data, - proc { +"" }, - method(:concat_javascript_sources) - end - end -end - -if Rails.env.local? - ActiveSupport.on_load(:action_view) do - def compute_asset_path(source, _options = {}) - "/assets/#{source}" - end - alias_method :public_compute_asset_path, :compute_asset_path - end -end - -# By default, the Sprockets DirectiveProcessor introduces a newline between possible 'header' comments -# and the rest of the JS file. (https://github.com/rails/sprockets/blob/f4d3dae71e/lib/sprockets/directive_processor.rb#L121) -# This causes sourcemaps to be offset by 1 line, and therefore breaks browser tooling. -# We know that Ember-Cli assets do not use Sprockets directives, so we can totally bypass the DirectiveProcessor for those files. -Sprockets::DirectiveProcessor.prepend( - Module.new do - def process_source(source) - return source, [] if EmberCli.is_ember_cli_asset?(File.basename(@filename)) - super - end - end, -) - -# Skip sprockets fingerprinting for some assets -Sprockets::Asset.prepend( - Module.new do - def digest_path - # Webpack chunks are already named based on their contents - return logical_path if logical_path.start_with?("chunk.") - super - end - end, -) - -# Sprockets::Cache::FileStore raises `Zlib::BufError: buffer error` sometimes. -# According to zlib documentation, Z_BUF_ERROR is not fatal and can be retried. -# https://www.zlib.net/zlib_faq.html#faq05 -Sprockets::Cache::FileStore.prepend( - Module.new do - def set(key, value) - attempts = 3 - - begin - attempts -= 1 - super - rescue Zlib::BufError - if attempts > 0 - puts "Zlib::BufError while deflating #{key}, retrying #{attempts} more times" - retry - else - raise - end - end - end - end, -) diff --git a/lib/middleware/turbo_dev.rb b/lib/middleware/turbo_dev.rb deleted file mode 100644 index 8754508caf6..00000000000 --- a/lib/middleware/turbo_dev.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true -module Middleware - # Cheat and bypass Rails in development mode if the client attempts to download a static asset - # that's already been downloaded. - # - # Also ensures that assets are not cached in development mode. Around Chrome 29, the behavior - # of `must-revalidate` changed and would often not request assets that had changed. - # - # To use, include in your project and add the following to development.rb: - # - # require 'middleware/turbo_dev' - # config.middleware.insert 0, Middleware::TurboDev - # - class TurboDev - def initialize(app, settings = {}) - @app = app - end - - def call(env) - root = "#{GlobalSetting.relative_url_root}/assets/" - is_asset = env["REQUEST_PATH"] && env["REQUEST_PATH"].starts_with?(root) - - # hack to bypass all middleware if serving assets, a lot faster 4.5 seconds -> 1.5 seconds - if (etag = env["HTTP_IF_NONE_MATCH"]) && is_asset - name = env["REQUEST_PATH"][(root.length)..-1] - etag = etag.gsub "\"", "" - asset = Rails.application.assets.find_asset(name) - return 304, {}, [] if asset && asset.digest == etag - end - - status, headers, response = @app.call(env) - headers["Cache-Control"] = "no-cache" if is_asset - [status, headers, response] - end - end -end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 4d993060e7c..9ba4585e7d8 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -814,9 +814,6 @@ class Plugin::Instance seed_data.each { |key, value| DiscoursePluginRegistry.register_seed_data(key, value) } - # Allow plugins to `register_asset` for images under /assets - Rails.configuration.assets.paths << File.dirname(path) + "/assets" - # Automatically include rake tasks Rake.add_rakelib(File.dirname(path) + "/lib/tasks") @@ -839,6 +836,7 @@ class Plugin::Instance end write_extra_js! + ensure_images_symlink! end def auth_provider(opts) @@ -1373,28 +1371,15 @@ class Plugin::Instance protected def self.js_path - File.expand_path "#{Rails.root}/app/assets/javascripts/plugins" - end - - def legacy_asset_paths - [ - "#{Plugin::Instance.js_path}/#{directory_name}.js.erb", - "#{Plugin::Instance.js_path}/#{directory_name}_extra.js.erb", - ] + File.expand_path "#{Rails.root}/app/assets/generated" end def extra_js_file_path - @extra_js_file_path ||= "#{Plugin::Instance.js_path}/#{directory_name}_extra.js" + @extra_js_file_path ||= + "#{Plugin::Instance.js_path}/#{directory_name}/plugins/#{directory_name}_extra.js" end def write_extra_js! - # No longer used, but we want to make sure the files are no longer present - # so they don't accidently get compiled by Sprockets. - legacy_asset_paths.each do |path| - File.delete(path) - rescue Errno::ENOENT - end - contents = javascript_includes.map { |js| File.read(js) } if contents.present? @@ -1408,6 +1393,21 @@ class Plugin::Instance end end + def ensure_images_symlink! + link_from = "#{Rails.root}/app/assets/generated/#{directory_name}/images" + link_target = "#{directory}/assets/images" + + if Dir.exist? link_target + ensure_directory(link_from) + Discourse::Utils.atomic_ln_s(link_target, link_from) + else + begin + File.delete(link_from) + rescue Errno::ENOENT + end + end + end + def register_assets! assets.each do |asset, opts, plugin_directory_name| DiscoursePluginRegistry.register_asset(asset, opts, plugin_directory_name) diff --git a/lib/tasks/assets.rake b/lib/tasks/assets.rake index 3679efc4da5..a869c93305b 100644 --- a/lib/tasks/assets.rake +++ b/lib/tasks/assets.rake @@ -1,11 +1,5 @@ # frozen_string_literal: true -task "assets:precompile:prereqs" do - if %w[profile production].exclude? Rails.env - raise "rake assets:precompile should only be run in RAILS_ENV=production, you are risking unminified assets" - end -end - task "assets:precompile:build" do if ENV["SKIP_EMBER_CLI_COMPILE"] != "1" ember_version = ENV["EMBER_VERSION"] || "5" @@ -29,44 +23,9 @@ task "assets:precompile:build" do end end -task "assets:precompile:before": %w[ - environment - assets:precompile:prereqs - assets:precompile:build - ] do - require "open3" - - # Ensure we ALWAYS do a clean build - # We use many .erbs that get out of date quickly, especially with plugins - STDERR.puts "Purging temp files" - `rm -fr #{Rails.root}/tmp/cache` - - Rails.configuration.assets.js_compressor = nil - Rails.configuration.assets.gzip = false - - STDERR.puts "Bundling assets" - - # in the past we applied a patch that removed asset postfixes, but it is terrible practice - # leaving very complicated build issues - # https://github.com/rails/sprockets-rails/issues/49 - - require "sprockets" - require "digest/sha1" -end +task "assets:precompile:before": %w[environment assets:precompile:build] task "assets:precompile:css" => "environment" do - class Sprockets::Manifest - def reload - @filename = find_directory_manifest(@directory) - @data = json_decode(File.read(@filename)) - end - end - - # cause on boot we loaded a blank manifest, - # we need to know where all the assets are to precompile CSS - # cause CSS uses asset_path - Rails.application.assets_manifest.reload - if ENV["DONT_PRECOMPILE_CSS"] == "1" || ENV["SKIP_DB_AND_REDIS"] == "1" STDERR.puts "Skipping CSS precompilation, ensure CSS lives in a shared directory across hosts" else @@ -109,22 +68,9 @@ def assets_path "#{Rails.root}/public/assets" end -def global_path_klass - @global_path_klass ||= Class.new { extend GlobalPath } -end - -def cdn_path(p) - global_path_klass.cdn_path(p) -end - -def cdn_relative_path(p) - global_path_klass.cdn_relative_path(p) -end - def gzip(path) - STDERR.puts "gzip -f -c -9 #{path} > #{path}.gz" - STDERR.puts `gzip -f -c -9 #{path} > #{path}.gz`.strip - raise "gzip compression failed: exit code #{$?.exitstatus}" if $?.exitstatus != 0 + cmd = "gzip -f -c -9 #{path} > #{path}.gz" + system cmd, exception: true end def brotli_command(path) @@ -133,11 +79,7 @@ def brotli_command(path) end def brotli(path) - STDERR.puts brotli_command(path) - STDERR.puts `#{brotli_command(path)}` - raise "brotli compression failed: exit code #{$?.exitstatus}" if $?.exitstatus != 0 - STDERR.puts `chmod +r #{path}.br`.strip - raise "chmod failed: exit code #{$?.exitstatus}" if $?.exitstatus != 0 + system brotli_command(path), exception: true end def concurrent? @@ -169,43 +111,35 @@ def log_task_duration(task_description, &task) end task "assets:precompile:compress_js": "environment" do - puts "Compressing Javascript and Generating Source Maps" - manifest = Sprockets::Manifest.new(assets_path) + puts "Compressing JavaScript files" - locales = Set.new(["en"]) - - RailsMultisite::ConnectionManagement.each_connection do |db| - locales.add(SiteSetting.default_locale) - end + load_path = Rails.application.assets.load_path log_task_duration("Done compressing all JS files") do concurrent? do |proc| - manifest - .files - .select { |k, v| k =~ /\.js\z/ } - .each do |file, info| - path = "#{assets_path}/#{file}" - if file.include? "discourse/tests" - STDERR.puts "Skipping: #{file}" - else - proc.call do - log_task_duration(file) do - STDERR.puts "Compressing: #{file}" + load_path + .assets + .select { |asset| asset.logical_path.extname == ".js" } + .each do |asset| + digested_path = asset.digested_path.to_s - info["size"] = File.size(path) - info["mtime"] = File.mtime(path).iso8601 - gzip(path) - brotli(path) - end + if digested_path.include? "discourse/tests" + STDERR.puts "Skipping: #{digested_path}" + next + end + + proc.call do + log_task_duration(digested_path) do + STDERR.puts "Compressing: #{digested_path}" + file_path = "public/assets/#{digested_path}" + gzip(file_path) + brotli(file_path) end end end end end - # protected - manifest.send :save - if GlobalSetting.fallback_assets_path.present? begin FileUtils.cp_r("#{Rails.root}/public/assets/.", GlobalSetting.fallback_assets_path) diff --git a/lib/tasks/s3.rake b/lib/tasks/s3.rake index 661eb360d14..6047e968984 100644 --- a/lib/tasks/s3.rake +++ b/lib/tasks/s3.rake @@ -51,38 +51,27 @@ def helper end def assets - cached = Rails.application.assets&.cached - manifest = - Sprockets::Manifest.new( - cached, - Rails.root + "public/assets", - Rails.application.config.assets.manifest, - ) + load_path = Rails.application.assets.load_path results = Set.new - manifest.assets.each do |_, path| - fullpath = (Rails.root + "public/assets/#{path}").to_s + load_path.assets.each do |asset| + fullpath = "#{Rails.root}/public/assets/#{asset.digested_path}" - # Ignore files we can't find the mime type of, like yarn.lock content_type = MiniMime.lookup_by_filename(fullpath)&.content_type content_type ||= "application/json" if fullpath.end_with?(".map") - if content_type - asset_path = "assets/#{path}" - results << [fullpath, asset_path, content_type] + next unless content_type - if File.exist?(fullpath + ".br") - results << [fullpath + ".br", brotli_s3_path(asset_path), content_type, "br"] - end + asset_path = "assets/#{asset.digested_path}" + results << [fullpath, asset_path, content_type] - if File.exist?(fullpath + ".gz") - results << [fullpath + ".gz", gzip_s3_path(asset_path), content_type, "gzip"] - end + if File.exist?(fullpath + ".br") + results << [fullpath + ".br", brotli_s3_path(asset_path), content_type, "br"] + end - if File.exist?(fullpath + ".map") - results << [fullpath + ".map", asset_path + ".map", "application/json"] - end + if File.exist?(fullpath + ".gz") + results << [fullpath + ".gz", gzip_s3_path(asset_path), content_type, "gzip"] end end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 67c0bdc86e4..04507e79ab5 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -62,7 +62,7 @@ RSpec.describe ApplicationHelper do it "deals correctly with subfolder" do set_subfolder "/community" expect(helper.preload_script("start-discourse")).to include( - "https://s3cdn.com/assets/start-discourse.js", + %r{https://s3cdn.com/assets/start-discourse-\w{8}.js}, ) end @@ -71,7 +71,7 @@ RSpec.describe ApplicationHelper do set_cdn_url "https://awesome.com" set_subfolder "/community" expect(helper.preload_script("start-discourse")).to include( - "https://s3cdn.com/s3_subpath/assets/start-discourse.js", + %r{https://s3cdn.com/s3_subpath/assets/start-discourse-\w{8}.js}, ) end @@ -79,68 +79,40 @@ RSpec.describe ApplicationHelper do helper.request.env["HTTP_ACCEPT_ENCODING"] = "br" link = helper.preload_script("start-discourse") - expect(link).to eq( - script_tag( - "https://s3cdn.com/assets/start-discourse.br.js", - "start-discourse", - helper.csp_nonce_placeholder, - ), - ) + expect(link).to include(%r{https://s3cdn.com/assets/start-discourse-\w{8}.br.js}) end it "gives s3 cdn if asset host is not set" do link = helper.preload_script("start-discourse") - expect(link).to eq( - script_tag( - "https://s3cdn.com/assets/start-discourse.js", - "start-discourse", - helper.csp_nonce_placeholder, - ), - ) + expect(link).to include(%r{https://s3cdn.com/assets/start-discourse-\w{8}.js}) end it "can fall back to gzip compression" do helper.request.env["HTTP_ACCEPT_ENCODING"] = "gzip" link = helper.preload_script("start-discourse") - expect(link).to eq( - script_tag( - "https://s3cdn.com/assets/start-discourse.gz.js", - "start-discourse", - helper.csp_nonce_placeholder, - ), - ) + expect(link).to include(%r{https://s3cdn.com/assets/start-discourse-\w{8}.gz.js}) end it "gives s3 cdn even if asset host is set" do set_cdn_url "https://awesome.com" link = helper.preload_script("start-discourse") - expect(link).to eq( - script_tag( - "https://s3cdn.com/assets/start-discourse.js", - "start-discourse", - helper.csp_nonce_placeholder, - ), - ) + expect(link).to include(%r{https://s3cdn.com/assets/start-discourse-\w{8}.js}) end it "gives s3 cdn but without brotli/gzip extensions for theme tests assets" do helper.request.env["HTTP_ACCEPT_ENCODING"] = "gzip, br" link = helper.preload_script("discourse/tests/theme_qunit_ember_jquery") - expect(link).to eq( - script_tag( - "https://s3cdn.com/assets/discourse/tests/theme_qunit_ember_jquery.js", - "discourse/tests/theme_qunit_ember_jquery", - helper.csp_nonce_placeholder, - ), + expect(link).to include( + %r{https://s3cdn.com/assets/discourse/tests/theme_qunit_ember_jquery-\w{8}.js}, ) end it "uses separate asset CDN if configured" do global_setting :s3_asset_cdn_url, "https://s3-asset-cdn.example.com" expect(helper.preload_script("start-discourse")).to include( - "https://s3-asset-cdn.example.com/assets/start-discourse.js", + %r{https://s3-asset-cdn.example.com/assets/start-discourse-\w{8}.js}, ) end end diff --git a/spec/lib/discourse_js_processor_spec.rb b/spec/lib/discourse_js_processor_spec.rb index 7bee91c5317..2d85f6dc7a2 100644 --- a/spec/lib/discourse_js_processor_spec.rb +++ b/spec/lib/discourse_js_processor_spec.rb @@ -3,23 +3,6 @@ require "discourse_js_processor" RSpec.describe DiscourseJsProcessor do - describe "should_transpile?" do - it "returns false for empty strings" do - expect(DiscourseJsProcessor.should_transpile?(nil)).to eq(false) - expect(DiscourseJsProcessor.should_transpile?("")).to eq(false) - end - - it "returns false for a regular js file" do - expect(DiscourseJsProcessor.should_transpile?("file.js")).to eq(false) - end - - it "returns true for deprecated .es6 files" do - expect(DiscourseJsProcessor.should_transpile?("file.es6")).to eq(true) - expect(DiscourseJsProcessor.should_transpile?("file.js.es6")).to eq(true) - expect(DiscourseJsProcessor.should_transpile?("file.js.es6.erb")).to eq(true) - end - end - describe "skip_module?" do it "returns false for empty strings" do expect(DiscourseJsProcessor.skip_module?(nil)).to eq(false) diff --git a/spec/lib/discourse_sourcemapping_url_processor_spec.rb b/spec/lib/discourse_sourcemapping_url_processor_spec.rb deleted file mode 100644 index 1948598d1fa..00000000000 --- a/spec/lib/discourse_sourcemapping_url_processor_spec.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -require "discourse_sourcemapping_url_processor" - -RSpec.describe DiscourseSourcemappingUrlProcessor do - def process(input) - env = Sprockets::Environment.new - env.context_class.class_eval do - def resolve(path, **kargs) - "/assets/mapped.js.map" - end - - def asset_path(path, options = {}) - "/assets/mapped-HEXGOESHERE.js.map" - end - end - - input = { environment: env, data: input, name: "mapped", filename: "mapped.js", metadata: {} } - DiscourseSourcemappingUrlProcessor.call(input)[:data] - end - - it "maintains relative paths" do - output = process "var mapped;\n//# sourceMappingURL=mapped.js.map" - expect(output).to eq("var mapped;\n//# sourceMappingURL=mapped-HEXGOESHERE.js.map\n//!\n") - end - - it "uses default behaviour for non-adjacent relative paths" do - output = process "var mapped;\n//# sourceMappingURL=/assets/mapped.js.map" - expect(output).to eq( - "var mapped;\n//# sourceMappingURL=/assets/mapped-HEXGOESHERE.js.map\n//!\n", - ) - end -end diff --git a/spec/services/push_notification_pusher_spec.rb b/spec/services/push_notification_pusher_spec.rb index 31220e3f3f5..c430693cd17 100644 --- a/spec/services/push_notification_pusher_spec.rb +++ b/spec/services/push_notification_pusher_spec.rb @@ -2,7 +2,9 @@ RSpec.describe PushNotificationPusher do it "returns badges url by default" do - expect(PushNotificationPusher.get_badge).to eq("/assets/push-notifications/discourse.png") + expect(PushNotificationPusher.get_badge).to match( + %r{\A/assets/push-notifications/discourse-\w{8}.png\z}, + ) end it "returns custom badges url" do @@ -52,12 +54,12 @@ RSpec.describe PushNotificationPusher do it "correctly guesses an image if missing" do message = execute_push(notification_type: -1) - expect(message[:icon]).to eq("/assets/push-notifications/discourse.png") + expect(message[:icon]).to match(%r{\A/assets/push-notifications/discourse-\w{8}.png\z}) end it "correctly finds image if exists" do message = execute_push(notification_type: 1) - expect(message[:icon]).to eq("/assets/push-notifications/mentioned.png") + expect(message[:icon]).to match(%r{\A/assets/push-notifications/mentioned-\w{8}.png\z}) end it "sends notification in user's locale" do