-
Notifications
You must be signed in to change notification settings - Fork 249
fix(builders): improve step bundle discovery and externalization for SDK serde classes #1669
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,9 @@ | ||
| --- | ||
| "@workflow/builders": patch | ||
| --- | ||
|
|
||
| Fix step bundle discovery and externalization for SDK serde classes | ||
|
|
||
| - Broaden `importParents` tracking to all imports (not just file extensions) so `parentHasChild()` works through bare specifier imports | ||
| - Include `workflow/runtime` in discovery inputs so SDK serde classes like `Run` are always discovered | ||
| - Bundle node_modules deps instead of externalizing with broken relative paths |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,7 +79,12 @@ export function createDiscoverEntriesPlugin( | |
| return { | ||
| name: 'discover-entries-esbuild-plugin', | ||
| setup(build) { | ||
| build.onResolve({ filter: jsTsRegex }, async (args) => { | ||
| // Track parent→child import relationships for ALL imports (not just | ||
| // those with file extensions) so that `parentHasChild()` can correctly | ||
| // identify transitive parents of serde/step files even when the | ||
| // dependency chain passes through bare specifier imports like | ||
| // `@workflow/core/runtime` or `workflow/runtime`. | ||
| build.onResolve({ filter: /.*/ }, async (args) => { | ||
|
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: Note Using
Member
Author
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. Good point. The discovery pass runs once (or once per watch rebuild) and the graph is bounded by the project's actual import tree, so this shouldn't be a bottleneck in practice. But agreed it's worth keeping an eye on for very large monorepos. |
||
| try { | ||
| const resolved = await enhancedResolve(args.resolveDir, args.path); | ||
|
|
||
|
Comment on lines
+82
to
90
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,6 +210,16 @@ export function createSwcPlugin(options: SwcPluginOptions): Plugin { | |
|
|
||
| let externalPath: string; | ||
| if (shouldMakeRelative) { | ||
| // When the resolved file lives inside node_modules, let | ||
| // esbuild bundle it rather than externalizing with a deeply | ||
| // nested relative path. Downstream bundlers (Rollup/Vite) | ||
| // can't rewrite opaque `__require()` calls in CJS shims, so | ||
| // relative paths computed for `outdir` break once the output | ||
| // is rebundled to a different directory. | ||
| if (normalizedResolvedPath.includes('/node_modules/')) { | ||
|
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: Note Bundling any resolved file under
Member
Author
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. Agreed. This is intentionally broad to fix the class of broken relative externals, but we should monitor step bundle sizes. In practice the node_modules files that hit this path are small CJS shims with internal require() calls (like |
||
| return null; // let esbuild bundle it | ||
| } | ||
|
|
||
| externalPath = relative( | ||
| options.outdir || process.cwd(), | ||
| resolvedPath | ||
|
|
||
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
Silently skipping when
workflow/runtimecannot be resolved preserves prior behavior, but it also means discovery still misses SDK-only serde files in misconfigured installs. If that scenario is worth surfacing, a debug log (or a more explicit warning mode) might help.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.
Fair point. The silent catch is intentional to avoid breaking builds when
workflowisn't installed yet (e.g. during initial setup), but a debug-level log would be helpful. Will add if this becomes a support issue.