Skip to content

Clean old sourcemap assets #201

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

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Clean old sourcemap assets #201

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/propshaft/output_path.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def all_files_from_tree(path)
end

def extract_path_and_digest(digested_path)
digest = digested_path.to_s[/-([0-9a-f]{7,128})\.(?!digested)[^.]+\z/, 1]
digest = digested_path.to_s[/-([0-9a-f]{7,128})\.(?!digested)/, 1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This regex should be the same as in server.rb:

propshaft/lib/propshaft/server.rb

Line 38 in af557fb

digest = full_path[/-([0-9a-zA-Z]{7,128})\.(?!digested)([^.]|.map)+\z/, 1]

Might be worth defining this as a constant on Propshaft::Asset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason why the server one shouldn't be the same as this one? The PR that introduced support for multiple extensions after .digested has made the same change to the regex made here https://github.com/rails/propshaft/pull/14/files#diff-cea8115669d1b6f7ed0b417671e6b929c07d4f41b4e009b82182221f38b46955L45

I agree a constant might be worth it, so when you change it, all places use the new version

Copy link
Contributor Author

@marcelolx marcelolx Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just not sure if we should use the same regex in all places (even though there a few different places are defining a regex like this )

path = digest ? digested_path.sub("-#{digest}", "") : digested_path

[path, digest]
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/output/one-f2e1ec15.txt.map
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
One from first path map
16 changes: 15 additions & 1 deletion test/propshaft/output_path_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ class Propshaft::OutputPathTest < ActiveSupport::TestCase
setup do
@manifest = {
".manifest.json": ".manifest.json",
"one.txt": "one-f2e1ec14.txt"
"one.txt": "one-f2e1ec14.txt",
"one.txt.map": "one-f2e1ec15.txt.map"
}.stringify_keys
@output_path = Propshaft::OutputPath.new(Pathname.new("#{__dir__}/../fixtures/output"), @manifest)
end
Expand Down Expand Up @@ -49,6 +50,19 @@ class Propshaft::OutputPathTest < ActiveSupport::TestCase
FileUtils.rm(current) if File.exist?(current)
end

test "clean keeps the correct number of versions regardless of the file extension" do
old = output_asset("by_count.txt.map", "old", created_at: Time.now - 300)
current = output_asset("by_count.txt.map", "current", created_at: Time.now - 180)

@output_path.clean(1, 0)

assert File.exist?(current)
assert_not File.exist?(old)
ensure
FileUtils.rm(old) if File.exist?(old)
FileUtils.rm(current) if File.exist?(current)
end

test "clean keeps all versions under a certain age" do
old = output_asset("by_age.txt", "old")
current = output_asset("by_age.txt", "current")
Expand Down