Skip to content

feat: export cliFetch util - #174

Merged
pi0 merged 3 commits into
mainfrom
feat/cli-fetch-util2
Jan 29, 2026
Merged

feat: export cliFetch util#174
pi0 merged 3 commits into
mainfrom
feat/cli-fetch-util2

Conversation

@pi0

@pi0 pi0 commented Jan 29, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes

    • CLI now exits with non-zero codes on failures and surfaces descriptive errors for non-OK responses.
  • Improvements

    • CLI options renamed for consistency (dir instead of cwd/base, entry instead of url); downstream mappings updated.
    • Verbose mode provides clearer request/response debugging using provided I/O streams.
    • Response streaming preserves binary/text encoding behavior.
  • New Features

    • Centralized CLI fetch path to standardize request construction, streaming, and error propagation.

✏️ Tip: You can customize this high-level summary in your review settings.

@pi0
pi0 requested a review from OskarLebuda January 29, 2026 21:28
@coderabbitai

coderabbitai Bot commented Jan 29, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Replaces inline CLI fetch with a new exported cliFetch that uses LoadOptions; renames loader options (urlentry, basedir); CLI now delegates fetch to cliFetch, uses try/catch to route stdout/stderr, and exits with code 0/1 based on success or thrown errors.

Changes

Cohort / File(s) Summary
CLI entrypoint & fetch
src/cli.ts
Adds exported cliFetch(options: LoadOptions & { url?: string; method?: string; headers?: string[]; data?: string; verbose?: boolean; hostname?: string; stdin?: NodeJS.ReadStream; stdout?: NodeJS.WriteStream; stderr?: NodeJS.WriteStream }). Replaces inlined fetch with delegation to cliFetch, adds try/catch-driven exit codes, streams response, and surfaces non-OK responses as thrown errors.
Loader API & types
src/loader.ts
Renames LoadOptions fields (urlentry, basedir), updates entry resolution/discovery to use entry and dir, and exports the updated LoadOptions type for public consumption.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • feat: loadServerEntry #168: Introduces or modifies the loader API and LoadOptions type used here; likely the predecessor or companion change to these renames and the loadServerEntry usage.

Poem

🐰 I hopped through code to shift a path,
entry and dir now do the math,
cliFetch carries header and stream,
verbose prints while night owls dream,
exit codes dance — a tidy craft.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the primary change: exporting a new cliFetch utility function. This is the main public API addition in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread src/cli.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/cli.ts (1)

157-185: Add duplex: "half" for streaming request bodies

When --data @- or `--data `@file is used, the body is a WHATWG ReadableStream. Node.js's fetch (via Undici) requires duplex: "half" for streaming request bodies; without it, new Request(...) throws TypeError: RequestInit: duplex option is required when sending a body.

Proposed fix
-    const req = new Request(url, {
-      method,
-      headers,
-      body,
-    });
+    const req = new Request(
+      url,
+      {
+        method,
+        headers,
+        body,
+        ...(body instanceof ReadableStream ? { duplex: "half" } : {}),
+      } as RequestInit & { duplex?: "half" },
+    );
src/loader.ts (1)

16-31: Handle file:// entries without breaking the notFound check

The docs say entry can be a URL, but resolve() + existsSync() will reject file://... inputs and prevent them from loading. The current code passes file URLs directly to resolve(), which treats them as path components, creating invalid paths like ./file:///path/to/server.ts. You should detect file URLs, convert them to paths using fileURLToPath for the existence check, and preserve the original URL for import.

🛠️ Proposed fix
-import { pathToFileURL } from "node:url";
+import { fileURLToPath, pathToFileURL } from "node:url";
-  let entry: string | undefined = opts.entry;
-  if (entry) {
-    entry = resolve(opts.dir || ".", entry);
-    if (!existsSync(entry)) {
-      return { notFound: true };
-    }
-  } else {
+  let entry: string | undefined = opts.entry;
+  let entryUrl: string | undefined;
+  if (entry) {
+    if (entry.startsWith("file://")) {
+      entryUrl = entry;
+      entry = fileURLToPath(entry);
+    } else {
+      entry = resolve(opts.dir || ".", entry);
+    }
+    if (!existsSync(entry)) {
+      return { notFound: true };
+    }
+  } else {
     for (const defEntry of defaultEntries) {
       for (const defExt of defaultExts) {
         const entryPath = resolve(opts.dir || ".", `${defEntry}${defExt}`);
         if (existsSync(entryPath)) {
           entry = entryPath;
           break;
         }
       }
       if (entry) break;
     }
     if (!entry) {
       return { notFound: true };
     }
   }
 
   // Convert to file:// URL for consistent imports
-  const url = entry.startsWith("file://") ? entry : pathToFileURL(resolve(entry)).href;
+  const url = entryUrl ?? pathToFileURL(entry!).href;

Also applies to: 90–115

@OskarLebuda OskarLebuda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM ❤️

@pi0
pi0 merged commit f75c354 into main Jan 29, 2026
9 of 12 checks passed
@pi0
pi0 deleted the feat/cli-fetch-util2 branch January 29, 2026 21:42
@codecov

codecov Bot commented Jan 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/loader.ts 0.00% 1 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants