Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve webpacker:clear task #2443

Merged
merged 1 commit into from Jan 26, 2020
Merged

Improve webpacker:clear task #2443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/tasks/webpacker/clean.rake
View file
Open in desktop
Expand Up @@ -4,9 +4,9 @@ require "webpacker/configuration"

namespace :webpacker do
desc "Remove old compiled webpacks"
task :clean, [:keep] => ["webpacker:verify_install", :environment] do |_, args|
task :clean, [:keep, :age] => ["webpacker:verify_install", :environment] do |_, args|
Webpacker.ensure_log_goes_to_stdout do
Webpacker.clean(Integer(args.keep || 2))
Webpacker.clean(Integer(args.keep || 2), Integer(args.age || 3600))
end
end
end
Expand Down
42 changes: 33 additions & 9 deletions lib/webpacker/commands.rb
View file
Open in desktop
Expand Up @@ -5,12 +5,34 @@ def initialize(webpacker)
@webpacker = webpacker
end

def clean(count = 2)
if config.public_output_path.exist? && config.public_manifest_path.exist? && versions.count > count
versions.drop(count).flat_map(&:last).each do |file|
File.delete(file) if File.file?(file)
logger.info "Removed #{file}"
end
# Cleanup old assets in the compile directory. By default it will
# keep the latest version, 2 backups created within the past hour.
Copy link

Choose a reason for hiding this comment

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

In what way is the age of the retained files important? This just causes asset availability downtime and doesn't include any explanation of why such a thing is desirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gauravtiwari @sbeam, maybe we can use the versions so long as they are OLDER than the age?

Right now, as soon as we find a few files with timestamps that differ by a second, we delete everything that's not in the current manifest.

#
# Examples
#
# To force only 1 backup to be kept, set count=1 and age=0.
#
# To only keep files created within the last 10 minutes, set count=0 and
# age=600.
Copy link
Contributor

Choose a reason for hiding this comment

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

@gauravtiwari if you really want to clean the directory, saving what Webpack created, why not just skip the whole clean and instead clear the destination directory before the build starts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was investigating this code today and I think those comments are misleading (or I'm mixing booleans in my head).

if we pass age as 0, then max_age will always be bigger, so the whole max_age < age && index < count will be false always and then everything will be removed independently of the value of index.
In this case, age should be something like Float::INFINITY

the same story for the second line, you can't pass count as 0, should be infinity too

#
def clean(count = 2, age = 3600)
if config.public_output_path.exist? && config.public_manifest_path.exist?
versions
.sort
.reverse
.each_with_index
.drop_while do |(mtime, _), index|
max_age = [0, Time.now - Time.at(mtime)].max
max_age < age && index < count
end
.each do |(_, files), index|
files.each do |file|
if File.file?(file)
File.delete(file)
logger.info "Removed #{file}"
end
end
end
end

true
Expand All @@ -37,14 +59,16 @@ def versions
manifest_config = Dir.glob("#{config.public_manifest_path}*")

packs = all_files - manifest_config - current_version
packs.group_by { |file| File.mtime(file).utc.to_i }.sort.reverse
packs.reject { |file| File.directory?(file) }.group_by { |file| File.mtime(file).utc.to_i }
Copy link

Choose a reason for hiding this comment

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

This still assumes that all the files for a given compilation run of webpack are going to be outputted within 1 second, and so can be grouped into distinct runs and counted that way. That's not a valid assumption.

The max_age code helps, but this still could lead to confusing and incorrect behavior. If files are older than the age param, they could still get grouped incorrectly and deleted prematurely.

Also, with default capistrano config or heroku buildpacks or docker deploys, users don't often have the option of (easily) changing the params given to rake assets:clean, or passing that param along to the webpack:clean task. So it's probably still important to make sure that the count method is reliable.

Copy link
Contributor

@justin808 justin808 Jan 27, 2020 •
edited

Choose a reason for hiding this comment

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

@sbeam see my comment above. How about cleaning before compilation, rather than after with all this logic?

Copy link

Choose a reason for hiding this comment

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

@justin808 that would work for some situations (including mine), but as I understand it, the point of keeping the last 2-3 versions of each compiled asses is in cases of rolling deploys or clients that have partially cached some of them, and there are still references to them out in the wild. So suddenly 404ing all old assets on each deploy isn't good. This is potentially an issue if you have e.g. capistrano deploys to multiple servers behind a naive load balancer that complete at different times.

That's why sprockets has similar code more or less for years.

Copy link

@sbeam sbeam Jan 27, 2020 •
edited

Choose a reason for hiding this comment

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

@justin808 if you do want to clean the dir entirely, I think you could use the clean webpack plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbeam I think for rails/webpacker, that can work. However, I generally prefer that the webpack config be solely focused on webpack builds and not things like deleting files from some directory. In other words, I find plain shell commands more descriptive. Another issue is that many React + React + SSR projects build the client and server bundles in the same directory. So some thought must be given the order of the webpack builds, if concurrency is used, and ensuring that the clean stuff is configured right.

end

def current_version
manifest.refresh.values.map do |value|
packs = manifest.refresh.values.map do |value|
next if value.is_a?(Hash)

File.join(config.root_path, "public", value)
File.join(config.root_path, "public", "#{value}*")
end.compact

Dir.glob(packs).uniq
end
end