Skip to content

fix(cli): import plugin artifacts through file URLs#1955

Open
logelog wants to merge 1 commit into
emdash-cms:mainfrom
logelog:fix/plugin-validator-windows-file-urls
Open

fix(cli): import plugin artifacts through file URLs#1955
logelog wants to merge 1 commit into
emdash-cms:mainfrom
logelog:fix/plugin-validator-windows-file-urls

Conversation

@logelog

@logelog logelog commented Jul 11, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes the current emdash plugin bundle / emdash plugin validate pipeline on Windows. The CLI was passing raw absolute artifact paths such as C:\\... to native import(), so Node interpreted the drive letter as an unsupported URL scheme.

Both generated-module imports now go through a small pathToFileURL(...).href helper. The helper has a cross-platform unit test that asserts an absolute artifact path becomes a file: specifier.

Related to #1236, which fixed the same class of bug in the earlier plugin CLI pipeline.

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). No user-visible strings are added.
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: not applicable (bug fix)

Targeted type-aware lint passed for all three changed TypeScript files; the full repository lint was not run.

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: OpenAI Codex (GPT-5)

Screenshots / test output

  • pnpm --filter emdash exec vitest run tests/unit/cli/bundle-utils.test.ts: 32 tests passed.
  • pnpm --filter "emdash..." build && pnpm --filter emdash typecheck: passed.
  • Targeted oxlint --type-aware --deny-warnings: 0 warnings, 0 errors.

@changeset-bot

changeset-bot Bot commented Jul 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 11af6b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/sandbox-workerd Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/do-demo-site Patch
@emdash-cms/do-solo-demo-site Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@swissky swissky added the bot:review Trigger an emdashbot code review on this PR label Jul 11, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the right fix for the stated problem. The CLI was importing built plugin artifacts with bare absolute paths, which fails on Windows because Node treats the drive letter as a URL scheme. Converting those paths via pathToFileURL(...).href in a small shared helper, then calling it from both import sites in bundle.ts, is the idiomatic fix. Since plugin validate is a thin wrapper around bundle --validateOnly, the fix covers the whole pipeline mentioned in the PR.

I checked:

  • The two dynamic file imports in bundle.ts now route through toFileImportSpecifier.
  • plugin-validate.ts delegates to the bundle command, so it inherits the fix.
  • Other CLI dynamic imports are package names or relative module specifiers and don’t need this treatment.
  • The runtime plugin loader uses data: URLs, not file paths, so it is unaffected.
  • The new helper and the updated import list follow the repo’s formatting (tabs, .js ESM extensions).
  • A changeset is present.
  • The unit test is a reasonable cross-platform smoke test that the output is a file: URL and differs from the raw path.

No blocking issues found. The PR is clean as-is.

@github-actions

Copy link
Copy Markdown
Contributor

Overlapping PRs

This PR modifies files that are also changed by other open PRs:

This may cause merge conflicts or duplicated work. A maintainer will coordinate.

@MA2153

MA2153 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Hello. Kindly sign the CLA. Thank you.

@logelog

logelog commented Jul 13, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Jul 13, 2026
@logelog logelog marked this pull request as ready for review July 13, 2026 23:14
@github-actions github-actions Bot added the review/approved Approved; no new commits since label Jul 13, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of #1955. The diff is unchanged since the prior (APPROVED) review, so this concentrates on re-verifying the claims rather than re-litigating settled points.

Approach: Still the right fix. On Windows, import("C:\\...\\index.mjs") fails because Node parses the drive letter as a URL scheme; routing the two built-artifact imports through pathToFileURL(path).href is the idiomatic, cross-platform fix. A thin shared helper in bundle-utils.ts keeps both call-sites consistent and unit-testable.

Re-verified:

  • Both file-path dynamic imports in bundle.ts (mainOutputPath and backendProbePath) go through toFileImportSpecifier; both inputs are absolute (produced via join(pluginDir, ...) from a resolved pluginDir, then findBuildOutput), so pathToFileURL handles them on every platform.
  • plugin validate delegates to bundleCommand via runCommand in plugin-validate.ts, and publish.ts likewise runs bundleCommand, so the whole bundle / validate / publish pipeline inherits the fix.
  • Other CLI dynamic imports (tsdown, node:zlib, node:fs/promises, citty, package specifiers, relative module specifiers) are not file paths and correctly need no conversion — no over-application.
  • The sibling pipeline in packages/plugin-cli/src/build/pipeline.ts already used inline pathToFileURL(...).href (the #1236 fix); this PR introduces the same technique via a helper, so the codebase stays consistent, no regression.
  • Changeset .changeset/calm-validator-paths.md uses "emdash": patch, matching the package name (packages/core/package.json"name": "emdash") and the format of sibling changesets.
  • The new unit test asserts the output is a file:-protocol URL and differs from the raw path; it runs identically on POSIX and Windows via pathToFileURL, so it genuinely guards the contract even though the original failure mode was Windows-only.
  • Formatting (tabs, .js ESM import extensions) matches repo convention.

No blocking or non-blocking issues found. The PR is clean as-is; the prior approval stands.

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

Labels

area/core bot:review Trigger an emdashbot code review on this PR cla: signed overlap review/approved Approved; no new commits since size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants