feat: support semver #145
feat: support semver #145
Conversation
WalkthroughAdds semver as a dependency and refactors version resolution to support semver ranges. getDownloadUrl is now async and resolves ranged selectors by querying and caching available Bun versions. New helpers for fetching/filtering versions added and a comprehensive test suite for selector behavior included. Changes
Pre-merge checks✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I already implemented something like that but Jarred closed it - #93 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)src/action.ts(4 hunks)tests/action.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/action.spec.ts (1)
src/action.ts (3)
getAllVersions(229-237)Input(21-30)options(49-126)
src/action.ts (1)
src/bunfig.ts (1)
writeBunfig(17-81)
🔇 Additional comments (3)
src/action.ts (2)
19-19: Import looks goodESM-compatible namespace import works with esbuild CJS output and semver v6+.
53-53: Switch to async URL resolution LGTMMoving to
await getDownloadUrl(options)cleanly integrates range resolution without changing downstream flow.package.json (1)
31-32: Corrected: semver v7 still requires @types/semversemver v7 does not ship built-in TypeScript types; the @types/semver package is required for both v6 and v7. The suggestion to drop @types/semver when upgrading is incorrect. If you choose to upgrade to semver v7 (currently 7.7.2), keep @types/semver aligned accordingly. If staying on v6, consider pinning with a caret (^6.x.x) or exact patch to reduce version drift.
Likely an incorrect or invalid review comment.
| let cachedVersions: semver.SemVer[] | null = null; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Version list cache: consider optional invalidation/TTL
Static cache is fine per-run. If long-lived processes or tests ever need refresh, expose a lightweight resetVersionsCache() or add a short TTL.
let cachedVersions: semver.SemVer[] | null = null;
+
+// Optional: testing/ops hook
+export function resetVersionsCache(): void {
+ cachedVersions = null;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cachedVersions: semver.SemVer[] | null = null; | |
| let cachedVersions: semver.SemVer[] | null = null; | |
| // Optional: testing/ops hook | |
| export function resetVersionsCache(): void { | |
| cachedVersions = null; | |
| } |
🤖 Prompt for AI Agents
In src/action.ts around lines 47-48 the versions cache is a module-level
variable (let cachedVersions: semver.SemVer[] | null = null;) with no way to
invalidate it; add a lightweight exported function resetVersionsCache() that
sets cachedVersions = null so callers/tests can force refresh, and optionally
add an exported configureVersionsCacheTTL(ms: number) or internal timestamp
check so the code invalidates the cache after a short TTL; update any code that
reads cachedVersions to respect TTL or call resetVersionsCache where
appropriate.
| export async function getDownloadUrl(options: Input): Promise<string> { | ||
| const { customUrl } = options; | ||
| if (customUrl) { | ||
| return customUrl; | ||
| } | ||
| const { version, os, arch, avx2, profile } = options; | ||
| const eversion = encodeURIComponent(version ?? "latest"); | ||
| let resolvedVersion: string | null = null; | ||
| const range = semver.validRange(version); | ||
| if (version == "") { | ||
| resolvedVersion = null; | ||
| } else if (range !== null) { | ||
| const availableVersions = await getAllVersions(); | ||
| const satisfiedVersion = semver.maxSatisfying(availableVersions, range); | ||
| if (satisfiedVersion === null) { | ||
| throw new Error(`Version "${version}" is not available`); | ||
| } | ||
| resolvedVersion = satisfiedVersion.toString(); | ||
| } else { | ||
| resolvedVersion = version; | ||
| } | ||
| const eversion = encodeURIComponent(resolvedVersion ?? "latest"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Harden range parsing: trim input and use strict equality
Trim whitespace to avoid surprises and use strict equality for the empty-string case. Optionally pass { loose: true } to validRange to be forgiving with user input.
-export async function getDownloadUrl(options: Input): Promise<string> {
+export async function getDownloadUrl(options: Input): Promise<string> {
const { customUrl } = options;
if (customUrl) {
return customUrl;
}
const { version, os, arch, avx2, profile } = options;
- let resolvedVersion: string | null = null;
- const range = semver.validRange(version);
- if (version == "") {
+ let resolvedVersion: string | null = null;
+ const raw = typeof version === "string" ? version.trim() : version;
+ const range = raw ? semver.validRange(raw, { loose: true }) : null;
+ if (raw === "") {
resolvedVersion = null;
} else if (range !== null) {
const availableVersions = await getAllVersions();
const satisfiedVersion = semver.maxSatisfying(availableVersions, range);
if (satisfiedVersion === null) {
- throw new Error(`Version "${version}" is not available`);
+ throw new Error(`Version "${raw}" is not available`);
}
resolvedVersion = satisfiedVersion.toString();
} else {
- resolvedVersion = version;
+ resolvedVersion = raw ?? undefined;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/action.ts around lines 161 to 181, the version input should be trimmed
and compared with strict equality to harden range parsing: create a
trimmedVersion = version.trim() (or destructure and trim when reading options),
use strict === "" when checking for empty string, and pass the trimmedVersion to
semver.validRange (optionally with { loose: true } if you want forgiving
parsing), then use trimmedVersion for subsequent logic (semver.maxSatisfying and
resolvedVersion assignments) so whitespace does not break range detection.
| export async function getAllVersions(): Promise<semver.SemVer[]> { | ||
| if (cachedVersions !== null) { | ||
| return cachedVersions; | ||
| } | ||
|
|
||
| const versions = await fetchAllVersions(); | ||
| cachedVersions = versions; | ||
| return versions; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
getAllVersions cache logic LGTM; consider minor ergonomics
Works as intended. If you add resetVersionsCache() (earlier comment), tests can force refresh without monkeypatching.
🤖 Prompt for AI Agents
In src/action.ts around lines 229 to 237, add and export a small helper to reset
the cached versions so tests can force a refresh: implement and export a
function named resetVersionsCache (or clearVersionsCache) that sets the
module-level cachedVersions back to null; keep it simple (no args, void return)
and use it in tests instead of monkeypatching to ensure fetchAllVersions is
called anew.
| async function fetchAllVersions(): Promise<semver.SemVer[]> { | ||
| const versions = await getExecOutput( | ||
| "npm", | ||
| ["view", "bun", "versions", "--json"], | ||
| { | ||
| silent: true, | ||
| ignoreReturnCode: true, | ||
| }, | ||
| ); | ||
|
|
||
| if (versions.exitCode == 0) { | ||
| const jsonList = JSON.parse(versions.stdout.trim()) as string[]; | ||
| const stableList = jsonList.filter((v) => !v.includes("-")); | ||
| const semverList = stableList.flatMap((v) => semver.coerce(v)); | ||
| const sortedList = semver.rsort(semverList); | ||
| return sortedList; | ||
| } | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Null-safe coerce and clearer failure signal from npm view
Guard against coerce returning null and add a warning when npm view fails to aid diagnostics. Returning an empty list conflates “no versions” with “fetch failed”.
if (versions.exitCode == 0) {
- const jsonList = JSON.parse(versions.stdout.trim()) as string[];
- const stableList = jsonList.filter((v) => !v.includes("-"));
- const semverList = stableList.flatMap((v) => semver.coerce(v));
- const sortedList = semver.rsort(semverList);
+ const jsonList = JSON.parse(versions.stdout.trim()) as string[];
+ const stableList = jsonList.filter((v) => !v.includes("-"));
+ const semverList = stableList
+ .map((v) => semver.coerce(v))
+ .filter((v): v is semver.SemVer => v !== null);
+ const sortedList = semver.rsort(semverList);
return sortedList;
}
- return [];
+ warning?.("Failed to retrieve versions via `npm view bun versions --json`; proceeding with empty list");
+ return [];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/action.ts around lines 239 to 258, make fetchAllVersions null-safe and
signal failure explicitly: change the return type to Promise<semver.SemVer[] |
null>, and when getExecOutput returns a non-zero exitCode call a logger/warning
(include exitCode and versions.stderr or stdout) and return null instead of an
empty array; when parsing results, guard semver.coerce by mapping and filtering
out nulls (e.g., flatMap/coerce then filter(Boolean)) before sorting so you
never pass nulls into semver.rsort.
| function getOptions(version: string | null): action.Input { | ||
| const options: action.Input = { | ||
| version: version, | ||
| }; | ||
| return options; | ||
| } |
There was a problem hiding this comment.
Broaden getOptions type to accept undefined (used by tests)
expectSuccess(undefined, "latest") passes undefined but getOptions only admits string | null. Add undefined to the parameter type.
-function getOptions(version: string | null): action.Input {
+function getOptions(version: string | null | undefined): action.Input {
const options: action.Input = {
version: version,
};
return options;
}Also applies to: 228-228
🤖 Prompt for AI Agents
In tests/action.spec.ts around lines 164-169 (and also at line 228), the
getOptions helper currently types its parameter as string | null but tests pass
undefined; update the function signature to accept string | null | undefined for
the version parameter, and adjust any related call sites or type annotations if
needed so undefined values are allowed and compiled cleanly.
| it(`input ${input} - failure`, async () => { | ||
| const options = getOptions(input); | ||
| expect(action.getDownloadUrl(options)).rejects.toThrow( | ||
| `Version "${input}" is not available`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Await the rejection assertion to avoid false positives
Without awaiting .rejects, the test may pass even if the promise resolves. Await the assertion.
- it(`input ${input} - failure`, async () => {
+ it(`input ${input} - failure`, async () => {
const options = getOptions(input);
- expect(action.getDownloadUrl(options)).rejects.toThrow(
+ await expect(action.getDownloadUrl(options)).rejects.toThrow(
`Version "${input}" is not available`,
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it(`input ${input} - failure`, async () => { | |
| const options = getOptions(input); | |
| expect(action.getDownloadUrl(options)).rejects.toThrow( | |
| `Version "${input}" is not available`, | |
| ); | |
| }); | |
| it(`input ${input} - failure`, async () => { | |
| const options = getOptions(input); | |
| await expect(action.getDownloadUrl(options)).rejects.toThrow( | |
| `Version "${input}" is not available`, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In tests/action.spec.ts around lines 181 to 186, the test uses
expect(...).rejects without awaiting it, which can cause false positives; update
the test to await the rejection assertion by adding an await before
expect(action.getDownloadUrl(options)).rejects.toThrow(...), ensuring the async
assertion is actually executed and the test fails when the promise resolves.
| describe("getDownloadUrl", async () => { | ||
| spyOn(action, "getAllVersions").mockResolvedValue(getAllVersions()); |
There was a problem hiding this comment.
Remove unnecessary async on describe
describe body doesn’t await anything; keeping it sync avoids subtle ordering issues.
- describe("getDownloadUrl", async () => {
+ describe("getDownloadUrl", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("getDownloadUrl", async () => { | |
| spyOn(action, "getAllVersions").mockResolvedValue(getAllVersions()); | |
| describe("getDownloadUrl", () => { | |
| spyOn(action, "getAllVersions").mockResolvedValue(getAllVersions()); |
🤖 Prompt for AI Agents
In tests/action.spec.ts around lines 190-191, the describe callback is declared
async despite not awaiting anything; remove the async keyword from the describe
declaration so the suite runs synchronously (if any setup needs async behavior,
move awaits into beforeEach/it blocks or use beforeAll with async as needed).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
tests/action.spec.ts (4)
160-163: Good: null‑safety forsemver.coerceis implementedMapping then filtering out nulls is correct; this prevents
SemVer | nullfrom leaking into later logic.
166-171: Fix types: allowundefinedand avoid assigningnulltoInput.versionTests pass
null/undefined, butInput.versionisstring | undefined. Don’t assignnull; omit the key when nullish, and widen helper types.Apply this diff:
-function getOptions(version: string | null): action.Input { - const options: action.Input = { - version: version, - }; - return options; -} +function getOptions(version: string | null | undefined): action.Input { + const options: action.Input = {}; + if (version != null) { + options.version = version; + } + return options; +} -function expectSuccess(input: string | null, output: string) { +function expectSuccess(input: string | null | undefined, output: string) {Also applies to: 173-173
182-189: Await the rejection assertion to avoid false positivesWithout
await, the test may pass even if the promise resolves.Apply this diff:
it(`input ${input} - failure`, async () => { const options = getOptions(input); - expect(action.getDownloadUrl(options)).rejects.toThrow( + await expect(action.getDownloadUrl(options)).rejects.toThrow( `Version "${input}" is not available`, ); });
191-194: Remove unnecessaryasyncfromdescribeThe
describebody doesn’t await anything; keep it sync.Apply this diff:
-describe("action", () => { - describe("getDownloadUrl", async () => { +describe("action", () => { + describe("getDownloadUrl", () => {src/action.ts (3)
47-48: Add a small cache reset helper for tests and long‑lived processesExpose an explicit invalidation hook.
Apply this diff:
let cachedVersions: semver.SemVer[] | null = null; +export function resetVersionsCache(): void { + cachedVersions = null; +}
249-260: Use strict equality forexitCodeand warn when npm query failsImproves correctness and diagnostics when
npm viewdoes not succeed.Apply this diff:
- if (versions.exitCode == 0) { + if (versions.exitCode === 0) { const jsonList = JSON.parse(versions.stdout.trim()) as string[]; const stableList = jsonList.filter((v) => !v.includes("-")); const semverList = stableList .map((v) => semver.coerce(v)) .filter((v): v is semver.SemVer => v !== null); - const sortedList = semver.rsort(semverList); - return sortedList; + semverList.sort((a, b) => semver.rcompare(a, b)); + return semverList; } - return []; + warning( + `Failed to retrieve versions via "npm view bun versions --json" (exitCode ${versions.exitCode}). stderr: ${versions.stderr?.trim() || "<empty>"}`, + ); + return [];
167-181: Type bug and input hardening ingetDownloadUrl
resolvedVersionisstring | nullbut later assignedundefined; also== ""and no trim can mis-handle whitespace. Normalize input, use strict equality, and keepresolvedVersionasstring | undefined.Apply this diff:
- let resolvedVersion: string | null = null; - const range = semver.validRange(version); - if (version == "") { - resolvedVersion = null; - } else if (range !== null) { + let resolvedVersion: string | undefined; + const raw = typeof version === "string" ? version.trim() : version; + const range = raw ? semver.validRange(raw, { loose: true }) : null; + if (raw === "") { + resolvedVersion = undefined; + } else if (range !== null) { const availableVersions = await getAllVersions(); const satisfiedVersion = semver.maxSatisfying(availableVersions, range); if (satisfiedVersion === null) { - throw new Error(`Version "${version}" is not available`); + throw new Error(`Version "${raw}" is not available`); } resolvedVersion = satisfiedVersion.toString(); } else { - resolvedVersion = version; + resolvedVersion = raw ?? undefined; } - const eversion = encodeURIComponent(resolvedVersion ?? "latest"); + const eversion = encodeURIComponent(resolvedVersion ?? "latest");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/action.ts(4 hunks)tests/action.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/action.spec.ts (1)
src/action.ts (2)
getAllVersions(229-237)Input(21-30)
| async function fetchAllVersions(): Promise<semver.SemVer[]> { | ||
| const versions = await getExecOutput( | ||
| "npm", | ||
| ["view", "bun", "versions", "--json"], | ||
| { | ||
| silent: true, | ||
| ignoreReturnCode: true, | ||
| }, | ||
| ); | ||
|
|
||
| if (versions.exitCode == 0) { | ||
| const jsonList = JSON.parse(versions.stdout.trim()) as string[]; | ||
| const stableList = jsonList.filter((v) => !v.includes("-")); | ||
| const semverList = stableList | ||
| .map((v) => semver.coerce(v)) | ||
| .filter((v): v is semver.SemVer => v !== null); | ||
| const sortedList = semver.rsort(semverList); | ||
| return sortedList; |
There was a problem hiding this comment.
Return type mismatch: semver.rsort yields string[], but function promises SemVer[]
Sort the SemVer[] in-place using rcompare to preserve types.
Apply this diff:
- if (versions.exitCode == 0) {
+ if (versions.exitCode == 0) {
const jsonList = JSON.parse(versions.stdout.trim()) as string[];
const stableList = jsonList.filter((v) => !v.includes("-"));
const semverList = stableList
.map((v) => semver.coerce(v))
.filter((v): v is semver.SemVer => v !== null);
- const sortedList = semver.rsort(semverList);
- return sortedList;
+ semverList.sort((a, b) => semver.rcompare(a, b));
+ return semverList;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function fetchAllVersions(): Promise<semver.SemVer[]> { | |
| const versions = await getExecOutput( | |
| "npm", | |
| ["view", "bun", "versions", "--json"], | |
| { | |
| silent: true, | |
| ignoreReturnCode: true, | |
| }, | |
| ); | |
| if (versions.exitCode == 0) { | |
| const jsonList = JSON.parse(versions.stdout.trim()) as string[]; | |
| const stableList = jsonList.filter((v) => !v.includes("-")); | |
| const semverList = stableList | |
| .map((v) => semver.coerce(v)) | |
| .filter((v): v is semver.SemVer => v !== null); | |
| const sortedList = semver.rsort(semverList); | |
| return sortedList; | |
| async function fetchAllVersions(): Promise<semver.SemVer[]> { | |
| const versions = await getExecOutput( | |
| "npm", | |
| ["view", "bun", "versions", "--json"], | |
| { | |
| silent: true, | |
| ignoreReturnCode: true, | |
| }, | |
| ); | |
| if (versions.exitCode == 0) { | |
| const jsonList = JSON.parse(versions.stdout.trim()) as string[]; | |
| const stableList = jsonList.filter((v) => !v.includes("-")); | |
| const semverList = stableList | |
| .map((v) => semver.coerce(v)) | |
| .filter((v): v is semver.SemVer => v !== null); | |
| semverList.sort((a, b) => semver.rcompare(a, b)); | |
| return semverList; |
🤖 Prompt for AI Agents
In src/action.ts around lines 239 to 256, the code uses semver.rsort which
returns string[] causing a mismatch with the function's Promise<semver.SemVer[]>
return type; replace the call with an in-place sort of the semver.SemVer[] using
semver.rcompare (e.g. call semverList.sort(semver.rcompare)) and then return the
mutated semverList so the returned type remains SemVer[].
2 participants
Resolved #37
Should we match the exact version (like
1.3.1) instead of latest?