fix(cli): import plugin artifacts through file URLs#1955
Conversation
🦋 Changeset detectedLatest commit: 11af6b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
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.tsnow route throughtoFileImportSpecifier. plugin-validate.tsdelegates 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,
.jsESM 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.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
Hello. Kindly sign the CLA. Thank you. |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
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(mainOutputPathandbackendProbePath) go throughtoFileImportSpecifier; both inputs are absolute (produced viajoin(pluginDir, ...)from aresolvedpluginDir, thenfindBuildOutput), sopathToFileURLhandles them on every platform. plugin validatedelegates tobundleCommandviarunCommandinplugin-validate.ts, andpublish.tslikewise runsbundleCommand, so the wholebundle/validate/publishpipeline 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.tsalready used inlinepathToFileURL(...).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.mduses"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 viapathToFileURL, so it genuinely guards the contract even though the original failure mode was Windows-only. - Formatting (tabs,
.jsESM import extensions) matches repo convention.
No blocking or non-blocking issues found. The PR is clean as-is; the prior approval stands.
What does this PR do?
Fixes the current
emdash plugin bundle/emdash plugin validatepipeline on Windows. The CLI was passing raw absolute artifact paths such asC:\\...to nativeimport(), so Node interpreted the drive letter as an unsupported URL scheme.Both generated-module imports now go through a small
pathToFileURL(...).hrefhelper. The helper has a cross-platform unit test that asserts an absolute artifact path becomes afile:specifier.Related to #1236, which fixed the same class of bug in the earlier plugin CLI pipeline.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runTargeted type-aware lint passed for all three changed TypeScript files; the full repository lint was not run.
AI-generated code disclosure
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.oxlint --type-aware --deny-warnings: 0 warnings, 0 errors.