feat(runner): add .ts↔.js ESM fallback for flow module resolution#1385
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughA new Sequence Diagram(s)sequenceDiagram
participant main
participant parseAsync
participant flushAndExit
participant stdout
participant stderr
participant process
main->>parseAsync: run command
parseAsync-->>main: resolved or rejected
main->>flushAndExit: exit code
flushAndExit->>stdout: write empty buffer
flushAndExit->>stderr: write empty buffer
stdout-->>flushAndExit: flush callback
stderr-->>flushAndExit: flush callback
flushAndExit->>process: exit(code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
![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
🤖 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 `@src/shell/resolver/swapSourceExtension.ts`:
- Around line 16-22: Restrict swapSourceExtension to local file specifiers only,
since the current extension lookup in swapSourceExtension can rewrite
package-like imports as well as sibling files. Update the guard logic in
swapSourceExtension to allow only relative paths, absolute paths, and file: URLs
before checking extensionSwaps, and leave all other specifiers unchanged. Add a
regression test covering a package subpath such as pkg/file.js to verify it is
not swapped.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19eaf8bb-e1a4-4657-803c-b5e5561affb3
📒 Files selected for processing (5)
src/domains/runner/loadFlowDefault.tssrc/shell/resolver/registerFlowModuleResolver.test.tssrc/shell/resolver/registerFlowModuleResolver.tssrc/shell/resolver/swapSourceExtension.test.tssrc/shell/resolver/swapSourceExtension.ts
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shell/resolver/registerFlowModuleResolver.ts (1)
68-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
TODO $TICKET_IDtracking comment for this resolution fallback.This
registerHooks-based ESM extension-swap fallback is exactly the kind of workaround the repo conventions require tracking. There is noTODO $TICKET_IDcomment referencing a Linear follow-up. Please file a ticket and add the reference (e.g. aboveregisterFlowModuleResolver) so the fallback can be revisited/removed once bundles emit matching extensions.As per path instructions: "For hacky workarounds, file a follow-up ticket in Linear and add a
TODO $TICKET_IDcomment to track it (relevant because this PR introduces an ESM resolution fallback viamodule.registerHooks)."🤖 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/shell/resolver/registerFlowModuleResolver.ts` around lines 68 - 89, Add a tracking comment for the ESM extension-swap fallback in registerFlowModuleResolver by filing a Linear ticket and placing a TODO $TICKET_ID near the resolver setup (for example above registerFlowModuleResolver or the registerHooks call). Keep the comment specific to the flowResolveHook/registerHooks workaround so it is easy to find and remove once bundled extensions match native Node resolution.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/shell/resolver/registerFlowModuleResolver.ts`:
- Around line 68-89: Add a tracking comment for the ESM extension-swap fallback
in registerFlowModuleResolver by filing a Linear ticket and placing a TODO
$TICKET_ID near the resolver setup (for example above registerFlowModuleResolver
or the registerHooks call). Keep the comment specific to the
flowResolveHook/registerHooks workaround so it is easy to find and remove once
bundled extensions match native Node resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3727f34-eed3-4ce5-ad6f-17652a19ca32
📒 Files selected for processing (5)
.changeset/ts-js-module-resolution-fallback.mdpackage.jsonsrc/shell/resolver/registerFlowModuleResolver.tssrc/shell/resolver/swapSourceExtension.test.tssrc/shell/resolver/swapSourceExtension.ts
Michael Price (michael-pr)
deleted the
investigate-module-resolution-failures
branch
2 participants
Overview of Changes
Platform-generated flow bundles frequently import sibling utilities with a
.tsextension (e.g.import codeSnippets from "../../../utilities/code-snippets.ts"), but those modules ship compiled as.js. Native Node ESM (Node 24 type-stripping) resolves extensions literally and throwsERR_MODULE_NOT_FOUND— 301 of 465 flows in a representative bundle hit this. QA Wolf's cloud runtime tolerates the mismatch; native Node does not.A synchronous
module.registerHooks({ resolve })hook is now registered inloadFlowDefaultimmediately before the non-compiled native import. Only onERR_MODULE_NOT_FOUNDdoes it retry the sibling source extension (.ts↔.js,.mts↔.mjs,.cts↔.cjs) and return the first that resolves. Literal matches always win, nothing is converted on disk (users keep authoring.ts), there is zero overhead on the normal path, and it emits nothing. The hook no-ops whereregisterHooksis unavailable (Bun, Node < 22.15).Follow-up (out of scope): the compiled-binary path (
Bun.buildinbundleFlow/executorPlugin) needs the same swap as anonResolvefallback to cover binary distributions.Testing
No meaningful standalone manual check — the fallback activates transparently when a flow bundle imports
.tssiblings that only exist as.json disk. Unit tests cover the extension-swap helper and all hook paths (passthrough, retry-on-not-found, non-ENOENT rethrow, original-error rethrow when swap also fails, bare-specifier no-swap).Checklist