From 824b4ef8397828423d2ddda117bf27e365954961 Mon Sep 17 00:00:00 2001 From: Maximo Mussini Date: Mon, 12 Aug 2024 10:33:27 -0300 Subject: [PATCH] fix: remove `vite:clean` rake task as it can potentially break apps Vite plugins can create additional files, and some of them are never referenced in the manifest. As a result, cleaning files can only be done *safely* by doing it *before* building with Vite, otherwise it can potentially break an app. Deployment setups without a CDN would actually benefit from keeping previous builds. Otherwise, in SPA clients might request an asset or dynamic import chunk that was removed from the server upon deployment, remaining in a broken state until a full-page reload. Container-based deployments don't need `clean`, and they are one of the most common ways to deploy apps nowadays. Taking all of this into account, I've decided to remove this task from `vite_ruby`, preventing broken apps, and easing the maintenance burden of something that I've never used and most users don't need. Closes #438, #490, #404 --- docs/src/guide/deployment.md | 6 --- docs/src/guide/introduction.md | 4 +- test/commands_test.rb | 31 +--------------- test/engine_rake_tasks_test.rb | 6 --- test/rake_tasks_test.rb | 1 - vite_ruby/lib/tasks/vite.rake | 13 ------- vite_ruby/lib/vite_ruby/commands.rb | 57 ----------------------------- 7 files changed, 3 insertions(+), 115 deletions(-) diff --git a/docs/src/guide/deployment.md b/docs/src/guide/deployment.md index db05350d..bc3abb42 100644 --- a/docs/src/guide/deployment.md +++ b/docs/src/guide/deployment.md @@ -63,12 +63,6 @@ The following rake tasks are available: Called automatically whenever assets:precompile is called. -- vite:clean[keep,age] - - Remove previous Vite builds. - - Called automatically whenever assets:clean is called. - - vite:clobber Remove the Vite build output directory. diff --git a/docs/src/guide/introduction.md b/docs/src/guide/introduction.md index 53087cb3..aeecc4cb 100644 --- a/docs/src/guide/introduction.md +++ b/docs/src/guide/introduction.md @@ -69,8 +69,8 @@ Interested in hearing more? [Read an introduction __blog post__][blog post], [le ### 🚀 Integrated with assets:precompile - [Rake tasks] for building and cleaning Vite assets are [automatically integrated][deployment] - with assets:precompile and assets:clean, so deploying is straightforward. + [Rake tasks] for building and Vite assets are [automatically integrated][deployment] + with assets:precompile, so deploying is straightforward. ### 🏗 Auto-build when not running Vite diff --git a/test/commands_test.rb b/test/commands_test.rb index d8ccf53f..35ec4316 100644 --- a/test/commands_test.rb +++ b/test/commands_test.rb @@ -7,7 +7,7 @@ def test_bootstrap assert ViteRuby.bootstrap end - delegate :build, :build_from_task, :clean, :clean_from_task, :clobber, to: 'ViteRuby.commands' + delegate :build, :build_from_task, :clobber, to: 'ViteRuby.commands' def test_build_returns_success_status_when_stale stub_builder(stale: true, build_successful: true) { @@ -35,35 +35,6 @@ def test_build_returns_failure_status_when_stale } end - def test_clean - with_rails_env('test') { |config| - manifest = config.build_output_dir.join('.vite/manifest.json') - js_file = config.build_output_dir.join('assets/application.js') - - # Should not clean, the manifest does not exist. - ensure_output_dirs(config) - refute clean - - # Should not clean, the file is recent. - manifest.write('{}') - js_file.write('export {}') - assert clean_from_task(OpenStruct.new) - assert_path_exists manifest - assert_path_exists js_file - - # Should not clean if directly referenced. - manifest.write('{ "application.js": { "file": "assets/application.js" } }') - assert clean(keep_up_to: 0, age_in_seconds: 0) - assert_path_exists js_file - - # Should clean if we remove age restrictions. - manifest.write('{}') - assert clean(keep_up_to: 0, age_in_seconds: 0) - assert_path_exists config.build_output_dir - refute_path_exists js_file - } - end - def test_clobber with_rails_env('test') { |config| ensure_output_dirs(config) diff --git a/test/engine_rake_tasks_test.rb b/test/engine_rake_tasks_test.rb index a960bcbe..8aa0c748 100644 --- a/test/engine_rake_tasks_test.rb +++ b/test/engine_rake_tasks_test.rb @@ -42,12 +42,6 @@ def test_rake_tasks refute_path_exists app_ssr_dir.join('.vite/manifest.json') refute_path_exists app_ssr_dir.join('.vite/manifest-assets.json') - within_mounted_app { `bundle exec rake app:vite:clean` } - refute Dir.empty?(app_public_dir.join('assets')) # Still fresh - - within_mounted_app { `bundle exec rake app:vite:clean[0,0]` } - refute Dir.empty?(app_public_dir.join('assets')) # Still referenced in manifest - within_mounted_app { `bundle exec rake app:vite:clobber` } refute_path_exists app_public_dir end diff --git a/test/rake_tasks_test.rb b/test/rake_tasks_test.rb index 277043b4..70d3d58e 100644 --- a/test/rake_tasks_test.rb +++ b/test/rake_tasks_test.rb @@ -8,7 +8,6 @@ def test_rake_tasks output = Dir.chdir(path_to_test_app) { `rake -T` } assert_includes output, 'vite:build' assert_includes output, 'vite:build_ssr' - assert_includes output, 'vite:clean' assert_includes output, 'vite:clobber' assert_includes output, 'vite:install_dependencies' assert_includes output, 'vite:verify_install' diff --git a/vite_ruby/lib/tasks/vite.rake b/vite_ruby/lib/tasks/vite.rake index 65121d98..390fbf51 100644 --- a/vite_ruby/lib/tasks/vite.rake +++ b/vite_ruby/lib/tasks/vite.rake @@ -25,11 +25,6 @@ namespace :vite do ViteRuby.commands.build_from_task('--ssr') if ViteRuby.config.ssr_build_enabled end - desc 'Remove old bundles created by ViteRuby' - task :clean, [:keep, :age] => :'vite:verify_install' do |_, args| - ViteRuby.commands.clean_from_task(args) - end - desc 'Remove the build output directory for ViteRuby' task clobber: :'vite:verify_install' do ViteRuby.commands.clobber @@ -76,14 +71,6 @@ unless ENV['VITE_RUBY_SKIP_ASSETS_PRECOMPILE_EXTENSION'] == 'true' end end - unless Rake::Task.task_defined?('assets:clean') - desc 'Remove old compiled assets' - Rake::Task.define_task('assets:clean', [:keep, :age]) - end - Rake::Task['assets:clean'].enhance do |_, args| - Rake::Task['vite:clean'].invoke(*args.to_h.values) - end - if Rake::Task.task_defined?('assets:clobber') Rake::Task['assets:clobber'].enhance do Rake::Task['vite:clobber'].invoke diff --git a/vite_ruby/lib/vite_ruby/commands.rb b/vite_ruby/lib/vite_ruby/commands.rb index e1c0249c..381c7257 100644 --- a/vite_ruby/lib/vite_ruby/commands.rb +++ b/vite_ruby/lib/vite_ruby/commands.rb @@ -28,38 +28,6 @@ def clobber $stdout.puts "Removed vite cache and output dirs:\n\t#{ dirs.join("\n\t") }" end - # Public: Receives arguments from a rake task. - def clean_from_task(args) - ensure_log_goes_to_stdout { - clean(keep_up_to: Integer(args.keep || 2), age_in_seconds: Integer(args.age || 3600)) - } - end - - # Public: Cleanup old assets in the output directory. - # - # keep_up_to - Max amount of backups to preserve. - # age_in_seconds - Amount of time to look back in order to preserve them. - # - # NOTE: By default keeps the last version, or 2 if created in the past hour. - # - # Examples: - # To force only 1 backup to be kept: clean(1, 0) - # To only keep files created within the last 10 minutes: clean(0, 600) - def clean(keep_up_to: 2, age_in_seconds: 3600) - return false unless may_clean? - - versions - .each_with_index - .drop_while { |(mtime, _files), index| - max_age = [0, Time.now - Time.at(mtime)].max - max_age < age_in_seconds || index < keep_up_to - } - .each do |(_, files), _| - clean_files(files) - end - true - end - # Internal: Installs the binstub for the CLI in the appropriate path. def install_binstubs `bundle binstub vite_ruby --path #{ config.root.join('bin') }` @@ -130,31 +98,6 @@ def print_info def_delegators :@vite_ruby, :config, :builder, :manifest, :logger, :logger= - def may_clean? - config.build_output_dir.exist? && config.manifest_paths.any? - end - - def clean_files(files) - files.select { |file| File.file?(file) }.each do |file| - File.delete(file) - logger.info("Removed #{ file }") - end - end - - def versions - all_files = Dir.glob("#{ config.build_output_dir }/**/*") - entries = all_files - config.manifest_paths - files_referenced_in_manifests - entries.reject { |file| File.directory?(file) } - .group_by { |file| File.mtime(file).utc.to_i } - .sort.reverse - end - - def files_referenced_in_manifests - config.manifest_paths.flat_map { |path| - JSON.parse(path.read).map { |_, entry| entry['file'] } - }.compact.uniq.map { |path| config.build_output_dir.join(path).to_s } - end - def with_node_env(env) original = ENV['NODE_ENV'] ENV['NODE_ENV'] = env