fix(build): preserve ESM imports in published package#565
Conversation
Rewrite emitted relative ESM specifiers to explicit .js targets so packaged installs load correctly in OpenCode. Use the plugin tool subpath directly to avoid root-entry resolution failures and fix #564.
WalkthroughThis PR addresses ESM import specifier issues in the published package. A post-build script ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Greptile SummaryThis PR fixes a packaging issue where emitted TypeScript output in
Confidence Score: 4/5Safe to merge — the fix correctly addresses the missing-extension problem and all three validation steps (build, typecheck, test) are reported as passing. The rewrite script is straightforward and the regex patterns cover static imports, dynamic imports, re-exports, and side-effect imports. The one edge to watch is that an unresolvable specifier is returned unchanged without any diagnostic output, so a broken import could slip into the published dist silently. Everything else — the subpath import change, test mock update, and build wiring — looks correct and consistent. script/fix-esm-imports.mjs — specifically the silent fallback in resolveSpecifier when neither .js nor index.js can be found on disk. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["npm run build"] --> B["tsc -p tsconfig.build.json\n(emits dist/**/*.js)"]
B --> C["node script/fix-esm-imports.mjs"]
C --> D{"dist/ exists?"}
D -- No --> E["exit silently"]
D -- Yes --> F["listJsFiles(dist/)"]
F --> G["for each .js file → fixFile()"]
G --> H["apply 3 regex patterns\n(from / import() / side-effect)"]
H --> I{"specifier\nhas extension?"}
I -- Yes --> J["keep as-is"]
I -- No --> K{"targetPath.js\nexists?"}
K -- Yes --> L["append .js"]
K -- No --> M{"targetPath/index.js\nexists?"}
M -- Yes --> N["append /index.js"]
M -- No --> O["⚠ return unchanged\n(silent fallback)"]
L --> P["writeFile if changed"]
N --> P
J --> P
O --> P
P --> Q["console.log: Fixed N dist file(s)"]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
script/fix-esm-imports.mjs:23-25
Silent fallback on unresolved specifier — when neither `${specifier}.js` nor `${specifier}/index.js` exists on disk, the function returns the original extension-less specifier unchanged. The output file will silently contain an unresolvable import, which only fails at runtime in the consuming project rather than failing the build step where it would be easy to catch.
```suggestion
if (existsSync(`${targetPath}.js`)) return `${specifier}.js`
if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js`
console.warn(`[fix-esm-imports] could not resolve specifier: ${specifier} (from ${filePath})`)
return specifier
```
### Issue 2 of 2
script/fix-esm-imports.mjs:49-53
**Stateful `/g` regex applied per-file in a loop** — `importPatterns` is a module-level array of regex literals all carrying the `/g` flag. `String.prototype.replace()` resets `lastIndex` to 0 before iterating, so this is safe in practice — but it is an easy trap if someone later changes the loop to use `.exec()` or `.test()`. Moving the regex construction inside `fixFile` (or inside `main` per-run) would eliminate the statefulness entirely and make the intent clearer.
Reviews (1): Last reviewed commit: "fix(build): preserve ESM imports in publ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | ||
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | ||
| return specifier |
There was a problem hiding this comment.
${specifier}.js nor ${specifier}/index.js exists on disk, the function returns the original extension-less specifier unchanged. The output file will silently contain an unresolvable import, which only fails at runtime in the consuming project rather than failing the build step where it would be easy to catch.
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | |
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | |
| return specifier | |
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | |
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | |
| console.warn(`[fix-esm-imports] could not resolve specifier: ${specifier} (from ${filePath})`) | |
| return specifier |
Prompt To Fix With AI
This is a comment left during a code review.
Path: script/fix-esm-imports.mjs
Line: 23-25
Comment:
Silent fallback on unresolved specifier — when neither `${specifier}.js` nor `${specifier}/index.js` exists on disk, the function returns the original extension-less specifier unchanged. The output file will silently contain an unresolvable import, which only fails at runtime in the consuming project rather than failing the build step where it would be easy to catch.
```suggestion
if (existsSync(`${targetPath}.js`)) return `${specifier}.js`
if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js`
console.warn(`[fix-esm-imports] could not resolve specifier: ${specifier} (from ${filePath})`)
return specifier
```
How can I resolve this? If you propose a fix, please make it concise.| for (const pattern of importPatterns) { | ||
| output = output.replace(pattern, (match, prefix, specifier, suffix) => { | ||
| return `${prefix}${resolveSpecifier(filePath, specifier)}${suffix}` | ||
| }) | ||
| } |
There was a problem hiding this comment.
/g regex applied per-file in a loop — importPatterns is a module-level array of regex literals all carrying the /g flag. String.prototype.replace() resets lastIndex to 0 before iterating, so this is safe in practice — but it is an easy trap if someone later changes the loop to use .exec() or .test(). Moving the regex construction inside fixFile (or inside main per-run) would eliminate the statefulness entirely and make the intent clearer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: script/fix-esm-imports.mjs
Line: 49-53
Comment:
**Stateful `/g` regex applied per-file in a loop** — `importPatterns` is a module-level array of regex literals all carrying the `/g` flag. `String.prototype.replace()` resets `lastIndex` to 0 before iterating, so this is safe in practice — but it is an easy trap if someone later changes the loop to use `.exec()` or `.test()`. Moving the regex construction inside `fixFile` (or inside `main` per-run) would eliminate the statefulness entirely and make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugin/quota-fallback.test.ts (1)
31-46: 💤 Low valueConsider moving
vi.mockto the top level.Placing
vi.mockinsidebeforeAllworks for this dynamic-import pattern (the mock is registered beforeawait import("../plugin")in the same callback), but it deviates from Vitest's idiomatic top-level placement and can surprise future maintainers who expect hoisting semantics.The standard pattern for this use case is:
♻️ Proposed refactor
import { beforeAll, describe, expect, it, vi } from "vitest"; import type { HeaderStyle, ModelFamily } from "./accounts"; + +vi.mock("@opencode-ai/plugin/tool", () => ({ + tool: vi.fn(), +})); // ... type declarations ... beforeAll(async () => { - vi.mock("@opencode-ai/plugin/tool", () => ({ - tool: vi.fn(), - })); - const { __testExports } = await import("../plugin"); // ... });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin/quota-fallback.test.ts` around lines 31 - 46, Move the vi.mock call out of beforeAll to the test file's top-level so the mock is registered before any imports; keep the same mock factory for "@opencode-ai/plugin/tool" and then in beforeAll only dynamically import("../plugin") and extract resolveQuotaFallbackHeaderStyle, getHeaderStyleFromUrl, and resolveHeaderRoutingDecision from __testExports — remove the vi.mock from inside beforeAll to follow Vitest hoisting/idiomatic placement while preserving the existing dynamic-import pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/fix-esm-imports.mjs`:
- Around line 19-26: resolveSpecifier currently returns the original specifier
silently when neither `${targetPath}.js` nor `${targetPath}/index.js` exists,
which can leave broken ESM imports; modify resolveSpecifier to detect this
unresolved case and emit a clear warning (e.g., console.warn) that includes the
filePath, specifier, and attempted resolution targets so the build author is
alerted; keep existing behavior for specifiers with extensions (hasExtension)
and successful resolutions, and reference resolveSpecifier, hasExtension, path,
and existsSync when locating the change.
---
Nitpick comments:
In `@src/plugin/quota-fallback.test.ts`:
- Around line 31-46: Move the vi.mock call out of beforeAll to the test file's
top-level so the mock is registered before any imports; keep the same mock
factory for "@opencode-ai/plugin/tool" and then in beforeAll only dynamically
import("../plugin") and extract resolveQuotaFallbackHeaderStyle,
getHeaderStyleFromUrl, and resolveHeaderRoutingDecision from __testExports —
remove the vi.mock from inside beforeAll to follow Vitest hoisting/idiomatic
placement while preserving the existing dynamic-import pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e92a85a5-f30c-4f66-8d19-fedead0c39de
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonscript/fix-esm-imports.mjssrc/plugin.tssrc/plugin/quota-fallback.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (2)
package.json (1)
37-37: LGTM.Using
&&ensuresfix-esm-imports.mjsonly runs on a successful TypeScript build, andprepublishOnlyalready chains throughnpm run build, so the post-processing is automatically included innpm publish.src/plugin.ts (1)
2-2: ⚡ Quick winThe
@opencode-ai/plugin/toolsubpath export is available in version 0.15.30. The import statement is valid and will not produceERR_PACKAGE_PATH_NOT_EXPORTEDat runtime. No action required.> Likely an incorrect or invalid review comment.
| function resolveSpecifier(filePath, specifier) { | ||
| if (hasExtension(specifier)) return specifier | ||
|
|
||
| const targetPath = path.resolve(path.dirname(filePath), specifier) | ||
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | ||
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | ||
| return specifier | ||
| } |
There was a problem hiding this comment.
Silent passthrough leaves broken specifiers unfixed without any signal.
When neither ${specifier}.js nor ${specifier}/index.js resolves on disk, resolveSpecifier returns the original extension-less specifier unchanged. This means a build where TypeScript emits an import for a path that wasn't correctly compiled (e.g., missing output file, typo in source) will silently produce a broken ESM specifier in dist/ — with no build-time indication.
Adding a console.warn for unresolved specifiers makes the failure observable:
🛡️ Proposed fix
function resolveSpecifier(filePath, specifier) {
if (hasExtension(specifier)) return specifier
const targetPath = path.resolve(path.dirname(filePath), specifier)
if (existsSync(`${targetPath}.js`)) return `${specifier}.js`
if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js`
+ console.warn(`[fix-esm-imports] Could not resolve specifier "${specifier}" in ${filePath} — leaving unchanged`)
return specifier
}📝 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.
| function resolveSpecifier(filePath, specifier) { | |
| if (hasExtension(specifier)) return specifier | |
| const targetPath = path.resolve(path.dirname(filePath), specifier) | |
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | |
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | |
| return specifier | |
| } | |
| function resolveSpecifier(filePath, specifier) { | |
| if (hasExtension(specifier)) return specifier | |
| const targetPath = path.resolve(path.dirname(filePath), specifier) | |
| if (existsSync(`${targetPath}.js`)) return `${specifier}.js` | |
| if (existsSync(path.join(targetPath, "index.js"))) return `${specifier}/index.js` | |
| console.warn(`[fix-esm-imports] Could not resolve specifier "${specifier}" in ${filePath} — leaving unchanged`) | |
| return specifier | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/fix-esm-imports.mjs` around lines 19 - 26, resolveSpecifier currently
returns the original specifier silently when neither `${targetPath}.js` nor
`${targetPath}/index.js` exists, which can leave broken ESM imports; modify
resolveSpecifier to detect this unresolved case and emit a clear warning (e.g.,
console.warn) that includes the filePath, specifier, and attempted resolution
targets so the build author is alerted; keep existing behavior for specifiers
with extensions (hasExtension) and successful resolutions, and reference
resolveSpecifier, hasExtension, path, and existsSync when locating the change.
1 participant
Summary
Fixes #564.
The published package could fail to load in OpenCode because emitted relative ESM specifiers in
dist/**/*.jsomitted explicit file extensions. When the plugin failed to load, OpenCode fell back to the default Google provider and surfaced a misleadingGOOGLE_GENERATIVE_AI_API_KEYerror instead of using the Antigravity OAuth path.Changes
dist/**/*.jsto explicit.jsor/index.jstargetsbuildcommand after TypeScript compilationtoolfrom@opencode-ai/plugin/toolinstead of the package root to avoid a dependency root-entry resolution failureValidation
npm run buildnpm run typechecknpm testimport('./dist/index.js')npm packoutput by installing the tarball into a clean temporary project and importingopencode-antigravity-auth