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.

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

Carry over pin options when pinning an existing pin #274

Closed
wants to merge 8 commits into from
44 changes: 37 additions & 7 deletions lib/importmap/packager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,18 @@ def pin_for(package, url)
def vendored_pin_for(package, url)
filename = package_filename(package)
version = extract_package_version_from(url)

if "#{package}.js" == filename
%(pin "#{package}" # #{version})
else
%(pin "#{package}", to: "#{filename}" # #{version})
end
line_formatted_pin_options = pin_options_for_package(package).except("to").map { |option, value| %(#{option}: #{value.is_a?(String) ? %("#{value}") : value}) }
Copy link
Contributor Author

@vietqhoang vietqhoang Oct 24, 2024

Choose a reason for hiding this comment

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

An alternative to managing the to is to update the pin options key to with the new value instead of straight up excluding it first and then reintroducing it again in pin_components.

A benefit to the alternative is that it maintains the original order of the pin options. This makes it a little nicer to review changes on git (and Github) since the ordering is maintain and thus highlighted nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes made to preserve ordering, 1fca7d7

pin_components = [
%(pin "#{package}"),
(%(to: "#{filename}") unless "#{package}.js" == filename),
*line_formatted_pin_options
].compact

%(#{pin_components.join(", ")} # #{version})
end

def packaged?(package)
importmap.match(/^pin ["']#{package}["'].*$/)
package_line_in_importmap(package).present?
end

def download(package, url)
Expand All @@ -62,6 +64,19 @@ def remove(package)
remove_package_from_importmap(package)
end

def pin_options_for_package(package)
line = package_line_in_importmap(package) || ""
raw_options = line.match(/^#{base_package_line_regex(package)}?,[\s+]?(?<pin_options>.*) #.*$/)
Copy link
Contributor Author

@vietqhoang vietqhoang Oct 24, 2024

Choose a reason for hiding this comment

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

I noticed the dummy app’s importmap.rb and some of the importmap.rb fixtures do not include the comment with the version for the pins. I think this may be due to how vendored pinning used to output? The regex may need to be relaxed to capture these lines.


return {} if raw_options.blank?

raw_options[:pin_options].split(/,\s?/).each_with_object({}) do |option, hash|
match_data = option.match(/^(?<option_name>[^:]*):[\s+]?["']?(?<option_value>.*[^"'])["']?$/)

hash[match_data[:option_name]] = cast_option_value(match_data[:option_value])
end
end

private
def post_json(body)
Net::HTTP.post(self.class.endpoint, body.to_json, "Content-Type" => "application/json")
Expand Down Expand Up @@ -146,4 +161,19 @@ def package_filename(package)
def extract_package_version_from(url)
url.match(/@\d+\.\d+\.\d+/)&.to_a&.first
end

def package_line_in_importmap(package)
importmap.match(/^#{base_package_line_regex(package)}.*$/).try(:[], 0)
end

def base_package_line_regex(package)
/pin ["']#{package}["']/
end

def cast_option_value(object)
return true if object == "true"
return false if object == "false"

object
end
end
7 changes: 7 additions & 0 deletions test/fixtures/files/pins_with_various_options_importmap.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
pin_all_from "app/assets/javascripts"

pin "md5", to: "https://cdn.skypack.dev/md5", preload: true # 1.0.2
pin "not_there", to: "nowhere.js", preload: false # 1.9.1
pin "some_file" # 0.2.1
pin "another_file",to:'another_file.js' # @0.0.16
pin "random", random_option: "foobar", hello: "world" # 7.7.7
11 changes: 10 additions & 1 deletion test/packager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require "minitest/mock"

class Importmap::PackagerTest < ActiveSupport::TestCase
setup { @packager = Importmap::Packager.new(Rails.root.join("config/importmap.rb")) }
setup { @packager = Importmap::Packager.new(Rails.root.join("../fixtures/files/pins_with_various_options_importmap.rb")) }

test "successful import with mock" do
response = Class.new do
Expand Down Expand Up @@ -54,5 +54,14 @@ def code() "200" end
test "vendored_pin_for" do
assert_equal %(pin "react" # @17.0.2), @packager.vendored_pin_for("react", "https://cdn/[email protected]")
assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/[email protected]")
assert_equal %(pin "md5", preload: true # @2.1.3), @packager.vendored_pin_for("md5", "https://cdn/[email protected]")
assert_equal %(pin "random", random_option: "foobar", hello: "world" # @8.8.8), @packager.vendored_pin_for("random", "https://cdn/[email protected]")
end

test "pin_options_for_package" do
assert_equal ({ "to" => "https://cdn.skypack.dev/md5", "preload" => true }), @packager.pin_options_for_package('md5')
assert_equal ({ "to" => "nowhere.js", "preload" => false }), @packager.pin_options_for_package('not_there')
assert_equal ({ }), @packager.pin_options_for_package('some_file')
assert_equal ({ "to" => "another_file.js" }), @packager.pin_options_for_package('another_file')
end
end
Loading