fix(windows): convert path to file url

[craftzdog]

This pull request makes a minor update to the way the package.json file is imported in the CommandPublish class. The change ensures the file path is properly converted to a file URL before importing, which improves compatibility and reliability.

• Updated CommandPublish to use pathToFileURL when importing package.json, ensuring correct file URL formatting. [1] [2]

@Keisir can you review it?

[claude]

Code Review

Summary

This is a correct and necessary fix for Windows compatibility. On Windows, absolute file paths use backslashes (e.g., C:\Users\user\package.json), which Node.js's dynamic import() cannot handle directly in ESM context — it requires a proper file:// URL. pathToFileURL from Node's built-in url module correctly converts the path, handling drive letters, backslashes, and special character encoding.

---

✅ What's good

• Correct fix: pathToFileURL is the idiomatic, Node.js-recommended way to convert a native path to a file URL for use with ESM import().
• No new dependencies: Uses Node's built-in url module.
• Minimal, focused change: Only touches the one line that needs fixing.
• Good commit message: Follows conventional commit format (fix(windows): ...).

---

🔍 Observations (not blocking)

1. Import caching caveat

Dynamic import() in Node.js caches modules by URL. If run() is called multiple times in the same process with different packagePath values pointing to different package.json files, subsequent calls could return a stale cached result from the first import.

This is unlikely to matter in practice for a CLI tool, but it's worth being aware of. An alternative that avoids this is:

import { readFile } from 'fs/promises'

const pkg = JSON.parse(
  await readFile(path.join(repoDir, 'package.json'), 'utf-8')
)

This also removes the need for the { with: { type: 'json' } } import attribute (which requires Node 20.10+ / 18.20+), making the code more broadly compatible.

2. Non-null assertion on repository (pre-existing)

At line 67, repository! uses a non-null assertion:

await this.uploadTarball(pkg, filePath, repository!, dryrun)

But getRepositoryId can return null, and validatePackageContents already throws if repository is null/invalid. This assertion is safe given the current flow, but it's a bit fragile — if validation order changes, the ! could mask a real null. A guard or explicit check here would be more defensive.

---

Verdict

LGTM — the fix is correct and should be merged. The observations above are minor and apply to pre-existing code rather than this PR's changes.

[claude]

PR Review: fix(windows): convert path to file url

Summary

The fix correctly addresses the Windows compatibility issue. Dynamic import() with a bare file path fails on Windows because ESM requires a file:// URL (e.g., file:///C:/path/to/package.json). Replacing it with readFile + JSON.parse sidesteps this entirely — a pragmatic and reliable solution.

---

Issues

Redundant readFile named import

src/commands/publish.ts, line 1:

import { readFile, rm } from 'fs/promises'  // readFile is redundant here
import * as fs from 'fs/promises'

readFile is already available as fs.readFile via the namespace import on line 2. The named import is a duplicate and should be removed. Use fs.readFile consistently with the rest of the file:

const pkg = JSON.parse(
  await fs.readFile(path.join(repoDir, 'package.json'), 'utf-8')
)

PR description is outdated

The description says "Updated CommandPublish to use pathToFileURL", but the final implementation does not use pathToFileURL at all — it uses readFile + JSON.parse. This appears to reflect the first commit, not the second. Worth updating the description to match the actual change.

---

Minor observations

• JSON parse errors: JSON.parse will throw a SyntaxError on malformed package.json. This is caught by the outer try/catch and logged, but the error message will just say "Failed to publish the package" with the raw SyntaxError. A small UX improvement would be wrapping the parse in a try/catch with a clearer message like "Failed to parse package.json", though this is not a blocker.
• The fix is otherwise clean — no unneeded complexity added.

---

Verdict

Approve after addressing the duplicate readFile import. The Windows fix itself is correct and well-scoped.

[claude]

Code Review

The PR fixes a Windows compatibility issue where dynamic import() with a bare file path fails because Windows paths are not valid module specifiers. Using fs.readFile + JSON.parse is a solid cross-platform fix.

Positive Changes

• Correct fix for Windows: import(path.join(...)) breaks on Windows since Node's ES module loader requires a proper file URL. JSON.parse + fs.readFile works everywhere.
• More compatible: import attributes require Node.js >= 20.10. The fs.readFile approach works on all Node versions.
• Cleaner imports: Removing the redundant named import for rm and using fs.rm consistently is a nice cleanup.

Issue 1 - PR title/description mismatch (minor)

The PR title says 'convert path to file url' and the description references pathToFileURL, but the final code does not use pathToFileURL. The commit history shows a two-step evolution (first tried pathToFileURL, then switched to readFile). Please update the title and description.

Suggested title: fix(windows): replace dynamic import with fs.readFile for package.json

Issue 2 - Missing error handling for malformed package.json

JSON.parse throws a SyntaxError if the file is malformed. The existing catch block only checks for ENOENT, so a malformed package.json produces a cryptic error. Consider wrapping the parse step with a clearer message like 'Failed to parse package.json -- ensure it contains valid JSON'.

Issue 3 - pkg: any typing (pre-existing, non-blocking)

pkg is typed as any throughout. Not introduced by this PR, but a minimal interface would improve compile-time type safety.

Verdict

The core fix is correct and well-motivated. Issues 1 and 2 are worth addressing but are not blockers. fs.readFile + JSON.parse is idiomatic, robust, and cross-platform compatible. Approved with suggestions.