-
Notifications
You must be signed in to change notification settings - Fork 263
fix(next): resolve bare specifiers in copied step files for lazy discovery #1670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| "@workflow/next": patch | ||
| "@workflow/builders": patch | ||
| --- | ||
|
|
||
| Fix lazy discovery bare specifier resolution in copied step files | ||
|
|
||
| - Use `enhanced-resolve` with ESM conditions to resolve bare specifiers from the original source file's location | ||
| - Only rewrite specifiers that can't resolve from the app directory (transitive SDK deps) | ||
| - Add `enhanced-resolve` to pnpm catalog and use `catalog:` in both packages |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| relative, | ||
| resolve, | ||
| } from 'node:path'; | ||
| import enhancedResolveOrig from 'enhanced-resolve'; | ||
| import { | ||
| createSocketServer, | ||
| type SocketIO, | ||
|
|
@@ -63,6 +64,45 @@ export async function getNextBuilderDeferred() { | |
| 'import("@workflow/builders")' | ||
| )) as typeof import('@workflow/builders'); | ||
|
|
||
| // Shared resolve options matching the configuration used by the SWC | ||
| // esbuild plugin (swc-esbuild-plugin.ts) for consistent resolution | ||
| // semantics across the toolchain. | ||
| const NODE_RESOLVE_OPTIONS = { | ||
| dependencyType: 'commonjs' as const, | ||
| modules: ['node_modules'], | ||
| exportsFields: ['exports'], | ||
| importsFields: ['imports'], | ||
| conditionNames: ['node', 'require'], | ||
| descriptionFiles: ['package.json'], | ||
| extensions: [ | ||
| '.ts', | ||
| '.tsx', | ||
| '.mts', | ||
| '.cts', | ||
| '.cjs', | ||
| '.mjs', | ||
| '.js', | ||
| '.jsx', | ||
| '.json', | ||
| '.node', | ||
| ], | ||
| enforceExtensions: false, | ||
| symlinks: true, | ||
| mainFields: ['main'], | ||
| mainFiles: ['index'], | ||
| roots: [], | ||
| fullySpecified: false, | ||
| preferRelative: false, | ||
| preferAbsolute: false, | ||
| restrictions: [], | ||
| }; | ||
|
|
||
| const NODE_ESM_RESOLVE_OPTIONS = { | ||
| ...NODE_RESOLVE_OPTIONS, | ||
| dependencyType: 'esm' as const, | ||
| conditionNames: ['node', 'import'], | ||
| }; | ||
|
|
||
| class NextDeferredBuilder extends BaseBuilderClass { | ||
| private socketIO?: SocketIO; | ||
| private readonly discoveredWorkflowFiles = new Set<string>(); | ||
|
|
@@ -74,6 +114,14 @@ export async function getNextBuilderDeferred() { | |
| private cacheWriteTimer: NodeJS.Timeout | null = null; | ||
| private deferredRebuildTimer: NodeJS.Timeout | null = null; | ||
| private lastDeferredBuildSignature: string | null = null; | ||
| // Lazily initialized resolvers for bare specifier rewriting. | ||
| // Cached to avoid re-creating on every import rewrite. | ||
| private esmSyncResolver?: ReturnType< | ||
| typeof enhancedResolveOrig.create.sync | ||
| >; | ||
| private cjsSyncResolver?: ReturnType< | ||
| typeof enhancedResolveOrig.create.sync | ||
| >; | ||
|
|
||
| async build() { | ||
| const outputDir = await this.findAppDirectory(); | ||
|
|
@@ -1267,6 +1315,33 @@ export async function getNextBuilderDeferred() { | |
| copiedStepFileBySourcePath: Map<string, string> | ||
| ): string { | ||
| if (!specifier.startsWith('.')) { | ||
| // Bare specifiers (e.g. '@workflow/serde') that are transitive | ||
| // dependencies of SDK packages can't be resolved by the bundler | ||
| // from the copied file's location (__workflow_step_files__/ inside | ||
| // the app dir) because the app doesn't directly depend on them. | ||
| // | ||
| // Only rewrite when the specifier can't be resolved from the app | ||
| // directory. If the package is a direct dependency of the app, | ||
| // the bare specifier will resolve normally and should be left as-is. | ||
| const appResolvable = this.resolveBareCopiedStepSpecifier( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI Review: Nit The comment refers to resolving from the app directory; |
||
| specifier, | ||
| copiedFilePath | ||
| ); | ||
| if (!appResolvable) { | ||
| const resolved = this.resolveBareCopiedStepSpecifier( | ||
| specifier, | ||
| sourceFilePath | ||
| ); | ||
| if (!resolved) return specifier; | ||
| let rewrittenPath = relative( | ||
| dirname(copiedFilePath), | ||
| resolved | ||
| ).replace(/\\/g, '/'); | ||
| if (!rewrittenPath.startsWith('.')) { | ||
| rewrittenPath = `./${rewrittenPath}`; | ||
| } | ||
| return rewrittenPath; | ||
| } | ||
| return specifier; | ||
| } | ||
|
|
||
|
|
@@ -1300,6 +1375,41 @@ export async function getNextBuilderDeferred() { | |
| return `${rewrittenPath}${suffix}`; | ||
| } | ||
|
|
||
| /** | ||
| * Resolves a bare specifier (e.g. '@workflow/serde', 'workflow') to an | ||
| * absolute file path using ESM-compatible resolution semantics via | ||
| * `enhanced-resolve`. Tries ESM conditions first (`node`, `import`), | ||
| * falling back to CJS resolution if ESM fails. | ||
| */ | ||
| private resolveBareCopiedStepSpecifier( | ||
| specifier: string, | ||
| sourceFilePath: string | ||
| ): string | undefined { | ||
| if (!this.esmSyncResolver) { | ||
| this.esmSyncResolver = enhancedResolveOrig.create.sync( | ||
| NODE_ESM_RESOLVE_OPTIONS | ||
| ); | ||
| } | ||
| if (!this.cjsSyncResolver) { | ||
| this.cjsSyncResolver = | ||
| enhancedResolveOrig.create.sync(NODE_RESOLVE_OPTIONS); | ||
| } | ||
| const context = dirname(sourceFilePath); | ||
| try { | ||
| const resolved = this.esmSyncResolver(context, specifier); | ||
| if (resolved) return resolved; | ||
| } catch { | ||
| // ESM resolution failed, try CJS | ||
| } | ||
| try { | ||
| const resolved = this.cjsSyncResolver(context, specifier); | ||
| if (resolved) return resolved; | ||
| } catch { | ||
| // CJS resolution also failed | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| private resolveCopiedStepImportTargetPath(targetPath: string): string { | ||
| if (existsSync(targetPath)) { | ||
| return targetPath; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI Review: Nit
These
NODE_RESOLVE_OPTIONS/NODE_ESM_RESOLVE_OPTIONSblocks intentionally mirrorpackages/builders/src/swc-esbuild-plugin.ts. That is good for consistency today, but it can drift over time. A follow-up could centralize the object (for example exporting a helper from@workflow/builders) so the two call sites cannot diverge silently.