Carry over pin options when pinning an existing pin #274
Conversation
The prior implementation returned a `MatchData` object if there is a match or `nil` if there is none. Although the existence of the object is truthy, given the method is interrogative my expectation is it should explicitly return a Boolean. This is being changed beause I want to use the `importmap.match` to extract the line from the file and pull the pin options. This is set up for keeping things DRY and to keep the commits simple for ease of following along the changes.
This returns any options present for the pinned package. Variations of lines tested against the `raw_options` regex can be viewed here: https://rubular.com/r/RlOaaKZsbAXfEz Variation of options tested against the option regex can be viewed here: https://rubular.com/r/qPUodCavWU46GX The method will be used to repopulate the options when the pinned package is updated. For the test case I changed the set up to use a fixture. I found editting the dummy apps importmap file to break other tests. Instead of spending time reconciling those tests I went with creating a fixture to be used for the particular test file.
The method now preserves the pin option if it is present.
I found the `split` can take in a regular expression.
lib/importmap/packager.rb
Outdated
| 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}) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changes made to preserve ordering, 1fca7d7
See comment for context: rails#274 (comment)
| @@ -62,6 +63,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>.*) #.*$/) | |||
There was a problem hiding this comment.
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.
|
I see where this is coming from, but it's too complicated. It's too far into attempting to turn this into a full-blown package manager. The repinning options will stand out in the git commit anyway, and it'll be up to the developer to ensure those are corrected for. |
Closes #243
Problem
When repinning a package,
importmap pin some_package_name, the pin options are not carried over.For example if the following pin already exists:
And the command is executed to update the pin to the latest version,
0.1.0:The
importmap pinreplaces thefoobarpinned line with the following:When updating packages it requires additional effort by the developer to manually carry over the options.
Expectation
The pin options should carry over.
For example if the following pin already exists:
And the command is executed to update the pin to the latest version,
0.1.0:The
importmap pinreplaces thefoobarpinned line with the following:Solution
Implement a method which extract the pin options from the matching line. Update the
vendored_pin_forwhich is used by thepinrake task to output with the carried over pin options.