-
Notifications
You must be signed in to change notification settings - Fork 183
Add discovered serializable classes in all context modes #874
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,6 @@ | ||
| --- | ||
| "@workflow/swc-plugin": patch | ||
| "@workflow/builders": patch | ||
| --- | ||
|
|
||
| Add discovered serializable classes in all context modes |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,7 @@ export abstract class BaseBuilder { | |
| { | ||
| discoveredSteps: string[]; | ||
| discoveredWorkflows: string[]; | ||
| discoveredSerdeFiles: string[]; | ||
| } | ||
| > = new WeakMap(); | ||
|
|
||
|
|
@@ -101,6 +102,7 @@ export abstract class BaseBuilder { | |
| ): Promise<{ | ||
| discoveredSteps: string[]; | ||
| discoveredWorkflows: string[]; | ||
| discoveredSerdeFiles: string[]; | ||
| }> { | ||
| const previousResult = this.discoveredEntries.get(inputs); | ||
|
|
||
|
|
@@ -110,9 +112,11 @@ export abstract class BaseBuilder { | |
| const state: { | ||
| discoveredSteps: string[]; | ||
| discoveredWorkflows: string[]; | ||
| discoveredSerdeFiles: string[]; | ||
| } = { | ||
| discoveredSteps: [], | ||
| discoveredWorkflows: [], | ||
| discoveredSerdeFiles: [], | ||
| }; | ||
|
|
||
| const discoverStart = Date.now(); | ||
|
|
@@ -277,11 +281,24 @@ export abstract class BaseBuilder { | |
| }> { | ||
| // These need to handle watching for dev to scan for | ||
| // new entries and changes to existing ones | ||
| const { discoveredSteps: stepFiles, discoveredWorkflows: workflowFiles } = | ||
| await this.discoverEntries(inputFiles, dirname(outfile)); | ||
| const { | ||
| discoveredSteps: stepFiles, | ||
| discoveredWorkflows: workflowFiles, | ||
| discoveredSerdeFiles: serdeFiles, | ||
| } = await this.discoverEntries(inputFiles, dirname(outfile)); | ||
|
|
||
| // Include serde files that aren't already step files for cross-context class registration. | ||
| // Classes need to be registered in the step bundle so they can be deserialized | ||
| // when receiving data from workflows and serialized when returning data to workflows. | ||
| const stepFilesSet = new Set(stepFiles); | ||
| const serdeOnlyFiles = serdeFiles.filter((f) => !stepFilesSet.has(f)); | ||
|
|
||
| // log the step files for debugging | ||
| await this.writeDebugFile(outfile, { stepFiles, workflowFiles }); | ||
| await this.writeDebugFile(outfile, { | ||
| stepFiles, | ||
| workflowFiles, | ||
| serdeOnlyFiles, | ||
| }); | ||
|
|
||
| const stepsBundleStart = Date.now(); | ||
| const workflowManifest: WorkflowManifest = {}; | ||
|
|
@@ -301,32 +318,38 @@ export abstract class BaseBuilder { | |
| ); | ||
| }); | ||
|
|
||
| // Helper to create import statement from file path | ||
| const createImport = (file: string) => { | ||
| // Normalize both paths to forward slashes before calling relative() | ||
| // This is critical on Windows where relative() can produce unexpected results with mixed path formats | ||
| const normalizedWorkingDir = this.config.workingDir.replace(/\\/g, '/'); | ||
| const normalizedFile = file.replace(/\\/g, '/'); | ||
| // Calculate relative path from working directory to the file | ||
| let relativePath = relative(normalizedWorkingDir, normalizedFile).replace( | ||
| /\\/g, | ||
| '/' | ||
| ); | ||
| // Ensure relative paths start with ./ so esbuild resolves them correctly | ||
| if (!relativePath.startsWith('.')) { | ||
| relativePath = `./${relativePath}`; | ||
| } | ||
| return `import '${relativePath}';`; | ||
| }; | ||
|
|
||
| // Create a virtual entry that imports all files. All step definitions | ||
| // will get registered thanks to the swc transform. | ||
| const imports = stepFiles | ||
| .map((file) => { | ||
| // Normalize both paths to forward slashes before calling relative() | ||
| // This is critical on Windows where relative() can produce unexpected results with mixed path formats | ||
| const normalizedWorkingDir = this.config.workingDir.replace(/\\/g, '/'); | ||
| const normalizedFile = file.replace(/\\/g, '/'); | ||
| // Calculate relative path from working directory to the file | ||
| let relativePath = relative( | ||
| normalizedWorkingDir, | ||
| normalizedFile | ||
| ).replace(/\\/g, '/'); | ||
| // Ensure relative paths start with ./ so esbuild resolves them correctly | ||
| if (!relativePath.startsWith('.')) { | ||
| relativePath = `./${relativePath}`; | ||
| } | ||
| return `import '${relativePath}';`; | ||
| }) | ||
| .join('\n'); | ||
| const stepImports = stepFiles.map(createImport).join('\n'); | ||
|
|
||
| // Include serde-only files for class registration side effects | ||
| const serdeImports = serdeOnlyFiles.map(createImport).join('\n'); | ||
|
|
||
| const entryContent = ` | ||
| // Built in steps | ||
| import '${builtInSteps}'; | ||
| // User steps | ||
| ${imports} | ||
| ${stepImports} | ||
| // Serde files for cross-context class registration | ||
| ${serdeImports} | ||
|
Comment on lines
325
to
+352
|
||
| // API entrypoint | ||
| export { stepEntrypoint as POST } from 'workflow/runtime';`; | ||
|
|
||
|
|
@@ -467,35 +490,49 @@ export abstract class BaseBuilder { | |
| interimBundleCtx: esbuild.BuildContext; | ||
| bundleFinal: (interimBundleResult: string) => Promise<void>; | ||
| }> { | ||
| const { discoveredWorkflows: workflowFiles } = await this.discoverEntries( | ||
| inputFiles, | ||
| dirname(outfile) | ||
| ); | ||
| const { | ||
| discoveredWorkflows: workflowFiles, | ||
| discoveredSerdeFiles: serdeFiles, | ||
| } = await this.discoverEntries(inputFiles, dirname(outfile)); | ||
|
|
||
| // Include serde files that aren't already workflow files for cross-context class registration. | ||
| // Classes need to be registered in the workflow bundle so they can be deserialized | ||
| // when receiving data from steps or when serializing data to send to steps. | ||
| const workflowFilesSet = new Set(workflowFiles); | ||
| const serdeOnlyFiles = serdeFiles.filter((f) => !workflowFilesSet.has(f)); | ||
|
|
||
| // log the workflow files for debugging | ||
| await this.writeDebugFile(outfile, { workflowFiles }); | ||
| await this.writeDebugFile(outfile, { workflowFiles, serdeOnlyFiles }); | ||
|
|
||
| // Helper to create import statement from file path | ||
| const createImport = (file: string) => { | ||
| // Normalize both paths to forward slashes before calling relative() | ||
| // This is critical on Windows where relative() can produce unexpected results with mixed path formats | ||
| const normalizedWorkingDir = this.config.workingDir.replace(/\\/g, '/'); | ||
| const normalizedFile = file.replace(/\\/g, '/'); | ||
| // Calculate relative path from working directory to the file | ||
| let relativePath = relative(normalizedWorkingDir, normalizedFile).replace( | ||
| /\\/g, | ||
| '/' | ||
| ); | ||
| // Ensure relative paths start with ./ so esbuild resolves them correctly | ||
| if (!relativePath.startsWith('.')) { | ||
| relativePath = `./${relativePath}`; | ||
| } | ||
| return `import '${relativePath}';`; | ||
| }; | ||
|
|
||
| // Create a virtual entry that imports all workflow files | ||
| // The SWC plugin in workflow mode emits `globalThis.__private_workflows.set(workflowId, fn)` | ||
| // calls directly, so we just need to import the files (Map is initialized via banner) | ||
| const imports = workflowFiles | ||
| .map((file) => { | ||
| // Normalize both paths to forward slashes before calling relative() | ||
| // This is critical on Windows where relative() can produce unexpected results with mixed path formats | ||
| const normalizedWorkingDir = this.config.workingDir.replace(/\\/g, '/'); | ||
| const normalizedFile = file.replace(/\\/g, '/'); | ||
| // Calculate relative path from working directory to the file | ||
| let relativePath = relative( | ||
| normalizedWorkingDir, | ||
| normalizedFile | ||
| ).replace(/\\/g, '/'); | ||
| // Ensure relative paths start with ./ so esbuild resolves them correctly | ||
| if (!relativePath.startsWith('.')) { | ||
| relativePath = `./${relativePath}`; | ||
| } | ||
| return `import '${relativePath}';`; | ||
| }) | ||
| .join('\n'); | ||
| const workflowImports = workflowFiles.map(createImport).join('\n'); | ||
|
|
||
| // Include serde-only files for class registration side effects | ||
| const serdeImports = serdeOnlyFiles.map(createImport).join('\n'); | ||
|
|
||
| const imports = serdeImports | ||
| ? `${workflowImports}\n// Serde files for cross-context class registration\n${serdeImports}` | ||
| : workflowImports; | ||
|
|
||
| const bundleStartTime = Date.now(); | ||
| const workflowManifest: WorkflowManifest = {}; | ||
|
|
@@ -697,18 +734,54 @@ export const POST = workflowEntrypoint(workflowCode);`; | |
|
|
||
| const inputFiles = await this.getInputFiles(); | ||
|
|
||
| // Create a virtual entry that imports all files | ||
| const imports = inputFiles | ||
| // Discover serde files from the input files' dependency tree for cross-context class registration. | ||
| // Classes need to be registered in the client bundle so they can be serialized | ||
| // when passing data to workflows via start() and deserialized when receiving workflow results. | ||
| const { discoveredSerdeFiles } = await this.discoverEntries( | ||
| inputFiles, | ||
| outputDir | ||
| ); | ||
|
|
||
| // Identify serde files that aren't in the inputFiles (deduplicated) | ||
| const inputFilesNormalized = new Set( | ||
| inputFiles.map((f) => f.replace(/\\/g, '/')) | ||
| ); | ||
| const serdeOnlyFiles = discoveredSerdeFiles.filter( | ||
| (f) => !inputFilesNormalized.has(f) | ||
| ); | ||
|
|
||
| // Re-exports for input files (user's workflow/step definitions) | ||
| const reexports = inputFiles | ||
| .map((file) => `export * from '${file}';`) | ||
| .join('\n'); | ||
|
|
||
| // Side-effect imports for serde files not in inputFiles (for class registration) | ||
| const serdeImports = serdeOnlyFiles | ||
| .map((file) => { | ||
| const normalizedWorkingDir = this.config.workingDir.replace(/\\/g, '/'); | ||
| let relativePath = relative(normalizedWorkingDir, file).replace( | ||
| /\\/g, | ||
| '/' | ||
| ); | ||
| if (!relativePath.startsWith('.')) { | ||
| relativePath = `./${relativePath}`; | ||
| } | ||
| return `import '${relativePath}';`; | ||
| }) | ||
| .join('\n'); | ||
|
|
||
| // Combine: serde imports (for registration side effects) + re-exports | ||
| const entryContent = serdeImports | ||
| ? `// Serde files for cross-context class registration\n${serdeImports}\n${reexports}` | ||
| : reexports; | ||
|
|
||
| // Bundle with esbuild and our custom SWC plugin | ||
| const clientResult = await esbuild.build({ | ||
| banner: { | ||
| js: '// biome-ignore-all lint: generated file\n/* eslint-disable */\n', | ||
| }, | ||
| stdin: { | ||
| contents: imports, | ||
| contents: entryContent, | ||
| resolveDir: this.config.workingDir, | ||
| sourcefile: 'virtual-entry.js', | ||
| loader: 'js', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1416,6 +1416,59 @@ describe('e2e', () => { | |
| } | ||
| ); | ||
|
|
||
| test( | ||
| 'crossContextSerdeWorkflow - classes defined in step code are deserializable in workflow context', | ||
| { timeout: 60_000 }, | ||
| async () => { | ||
| // This is a critical test for the cross-context class registration feature. | ||
| // | ||
| // The Vector class is defined in serde-models.ts and ONLY imported by step code | ||
| // (serde-steps.ts). The workflow code (99_e2e.ts) does NOT import Vector directly. | ||
| // | ||
| // Without cross-context class registration, this test would fail because: | ||
| // - The workflow bundle wouldn't have Vector registered (never imported it) | ||
| // - The workflow couldn't deserialize Vector instances returned from steps | ||
| // | ||
| // With cross-context class registration: | ||
| // - The build system discovers serde-models.ts has serialization patterns | ||
| // - It includes serde-models.ts in ALL bundle contexts (step, workflow, client) | ||
| // - Vector is registered everywhere, enabling full round-trip serialization | ||
| // | ||
| // Test flow: | ||
| // 1. Step creates Vector(1, 2, 3) and returns it (step serializes) | ||
| // 2. Workflow receives Vector (workflow MUST deserialize - key test!) | ||
| // 3. Workflow passes Vector to another step (workflow serializes) | ||
| // 4. Step receives Vector and operates on it (step deserializes) | ||
| // 5. Workflow returns plain objects to client (no client deserialization needed) | ||
| // | ||
| // The critical part is step 2: the workflow code never imports Vector, | ||
| // so without cross-context registration it wouldn't know how to deserialize it. | ||
|
|
||
| const run = await triggerWorkflow('crossContextSerdeWorkflow', []); | ||
| const returnValue = await getWorkflowReturnValue(run.runId); | ||
|
|
||
| // Verify all the vector operations worked correctly | ||
| expect(returnValue).toEqual({ | ||
| // v1 created in step: (1, 2, 3) | ||
| v1: { x: 1, y: 2, z: 3 }, | ||
| // v2 created in step: (10, 20, 30) | ||
| v2: { x: 10, y: 20, z: 30 }, | ||
| // sum of v1 + v2: (11, 22, 33) | ||
| sum: { x: 11, y: 22, z: 33 }, | ||
| // v1 scaled by 5: (5, 10, 15) | ||
| scaled: { x: 5, y: 10, z: 15 }, | ||
| // Array sum of v1 + v2 + scaled: (16, 32, 48) | ||
| arraySum: { x: 16, y: 32, z: 48 }, | ||
| }); | ||
|
|
||
| // Verify the run completed successfully | ||
| const { json: runData } = await cliInspectJson( | ||
| `runs ${run.runId} --withData` | ||
| ); | ||
|
Comment on lines
1416
to
+1467
|
||
| expect(runData.status).toBe('completed'); | ||
| } | ||
| ); | ||
|
|
||
| // ==================== PAGES ROUTER TESTS ==================== | ||
| // Tests for Next.js Pages Router API endpoint (only runs for nextjs-turbopack and nextjs-webpack) | ||
| const isNextJsApp = | ||
|
|
||
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.
The createImport helper function is duplicated three times in this file (here at lines 320-335, at lines 503-518 for workflow bundle, and at lines 746-756 for client bundle). Consider extracting this into a private class method to improve maintainability and ensure consistency across all three bundle contexts. This would make the code DRY and easier to maintain if the path normalization logic needs to be updated in the future.