Skip to content

Commit

Permalink
fix: remove vite:clean rake task as it can potentially break apps
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@ElMassimo
ElMassimo committed Aug 12, 2024
1 parent ebe50b8 commit 824b4ef
Show file tree
Hide file tree
Showing 7 changed files with 3 additions and 115 deletions.
6 changes: 0 additions & 6 deletions docs/src/guide/deployment.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,6 @@ The following rake tasks are available:

Called automatically whenever <kbd>assets:precompile</kbd> is called.

- <kbd>vite:clean[keep,age]</kbd>

Remove previous Vite builds.

Called automatically whenever <kbd>assets:clean</kbd> is called.

- <kbd>vite:clobber</kbd>

Remove the Vite build output directory.
Expand Down
4 changes: 2 additions & 2 deletions docs/src/guide/introduction.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ Interested in hearing more? [Read an introduction __blog post__][blog post], [le

### 🚀 Integrated with <kbd>assets:precompile</kbd>

[Rake tasks] for building and cleaning Vite assets are [automatically integrated][deployment]
with <kbd>assets:precompile</kbd> and <kbd>assets:clean</kbd>, so deploying is straightforward.
[Rake tasks] for building and Vite assets are [automatically integrated][deployment]
with <kbd>assets:precompile</kbd>, so deploying is straightforward.

### 🏗 Auto-build when not running Vite

Expand Down
31 changes: 1 addition & 30 deletions test/commands_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 0 additions & 6 deletions test/engine_rake_tasks_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion test/rake_tasks_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
13 changes: 0 additions & 13 deletions vite_ruby/lib/tasks/vite.rake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
57 changes: 0 additions & 57 deletions vite_ruby/lib/vite_ruby/commands.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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') }`
Expand Down Expand Up @@ -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
Expand Down

2 comments on commit 824b4ef

@KieranP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElMassimo Is there a suggested path going forward for those who deploy to servers using Capistrano? Sprocket assets are being cleaned up, but vite isn't anymore, leading to growing public/vite/ folders each time we deploy.

@ElMassimo
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping older assets around for a while is a good practice, especially when using dynamic imports, as clients still using an older bundle of the app could request a file that is no longer available in the most recent bundle. Using a CDN mitigates that because once an asset is served it tends to stay available for a long time (or forever), but only if the referenced asset was requested at least once.

cleaning files can only be done safely by doing it before building with Vite, otherwise it can potentially break an app.

Depending on the plugins that your app is using, it might be safe to use a custom rake task that checks public/vite for files with a very old mtime, and delete those to release disk space.

Please sign in to comment.