fix(cli): pin --vendor leaves a pin to a custom URL alone - #41
Merged
Merged
Conversation
--vendor overrides the check that refuses a download; it does not name a destination. The docs already say so in as many words — "--remote wins; it names a destination, where --vendor only overrides a check" — but the code disagreed, because the branch that skips a custom URL runs only while vendor is false: pin "md5", to: "https://cdn.example.com/md5.js", preload: false $ bin/importmap pin md5@2.3.0 --vendor Pinning "md5" to vendor/javascript/md5.js via download from https://ga.jspm.io/… The URL the app had chosen was gone, replaced by a file from whichever CDN the spec happened to resolve to, and `vendor ||= packager.vendored?(package)` kept it that way on every later rewrite. Nothing was printed about it. The skip now happens whether or not --vendor was passed, and the sentence names the flag so the override doesn't look ignored. --from is left as the one way to move such a pin on purpose: it already overrules a custom URL in repin_remote_package, so the guard stands down when a CDN was named, and --vendor --from still vendors from that CDN. ## Test coverage - test/commands_test.rb: --vendor against a custom-URL pin prints the skip and leaves both config/importmap.rb and vendor/javascript untouched; --vendor --from jspm still vendors, so the escape hatch is pinned down too Closes #29
CHANGELOG.md was the only conflict: #40 and this branch each appended a bullet to 1.2.0's "### Fixed" at the same spot. Union, main's bullet first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bin/importmap pin <pkg> --vendoragainst a pin whoseto:is a host no provider answers for discarded the URL without a word and vendored the file from whichever CDN the spec resolved to.Commands#pin_packagereachesrepin_remote_package— the only place that printsSkipping "…" pinned to custom URL …— only whenexisting_url && !vendor && !regraph. With--vendorthat branch is skipped,--remoteis false, and control falls topin_vendored_package.vendor ||= packager.vendored?(package)then kept the pin that way on every later rewrite.The fix is one
elsifinpin_packageplus the sentence it prints:Docs updated:
docs/app/views/docs/pages/pinning.rb(Custom URLs now shows the--vendorskip and points at--from),docs/app/views/docs/pages/multi_file_packages.rb(the paragraph that already said--vendor"only overrides a check" now says what that means for a custom URL).CHANGELOG entry under
## 1.2.0→### Fixed.Closes #29
Test plan
bundle exec rake test— 433 runs, 0 failures, with bun installedmainit fails withPinning "md5" to vendor/javascript/md5.js via download from https://ga.jspm.io/…where the skip was expectedcd docs && bundle exec rake lint && bundle exec rspec— 53 files no offenses, 54 examples passed (the internal-links spec covers the new#custom-urlslink)pin "md5", to: "https://cdn.example.com/md5.js"in an app, thenbin/importmap pin md5 --vendor— line unchanged, nothing invendor/javascript;bin/importmap pin md5 --vendor --from jspm— vendoredDeviations & judgment calls
--vendor's documented job is overriding the single-file check, not choosing a source, and because a custom URL carries no version and no provider —vendored_pin_forwould have to invent a provenance comment shape (# @?with no CDN) that nothing else in the pin-line contract produces. That is a feature with a design of its own, not this bug's fix. The bug here is the silence, and the silence is what this removes. Happy to open a follow-up issue for "vendor the file the app actually pinned" if you want it.--fromstands the guard down.repin_remote_packagealready returns early onfrom, sopin md5 --from jspmmoves a custom-URL pin today. Making--vendorrefuse unconditionally would have left--vendor --from jspmrefusing where--from jspmalone works. A second test pins that behaviour down so the escape hatch can't rot.skip().skip()makespinexit 1; the existing custom-URL path justputsand exits 0. Matching the sibling was worth more than flagging it as a failure, andrun_importmap_commandwould have flunked on a non-zero exit anyway.commands_test.rbcase is justified here under.claude/rules/testing.md: this is a new printed sentence, which is one of the two things that earns a live test. Versions are pinned (md5@2.3.0,md5@2.2.0) so a new md5 release can't move the expectations.commands_test.rb, not next to their sibling at line 214, per the upstream-sync rule that keeps the merge cheap.mainindependently of fix(cli): read an array preload back whatever quotes it is written in #40 (issue Single-quoted array preload crashes extract_existing_pin_options; empty array is dropped #28); the two diffs don't overlap.Summary by cubic
Fixes #29 so
bin/importmap pin --vendorleaves a pin to a custom URL alone. Previously it silently replaced the pinned URL by vendoring from whichever CDN the spec resolved to; now it prints a skip message naming--fromand leavesimportmap.rbandvendor/javascriptuntouched.--vendor --from <cdn>still vendors from the named CDN.Written for commit a53fb16. Summary will update on new commits.