-
Notifications
You must be signed in to change notification settings - Fork 106
fix: normalize workbench tests #292
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 25 commits
87ce599
e532869
cd1888b
ce120f2
67fb677
2563978
ea58dc1
63d92ce
a59f764
63a9c4c
761278f
f30c295
4959f80
6f92685
f94a38b
e2ea115
482d0da
b181a03
353d877
06b916d
12a524d
00f3e51
4792d39
ef98862
47e7bfb
a38c472
6b5d113
b6d2d7c
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,5 @@ | ||
| --- | ||
| "@workflow/nitro": patch | ||
| --- | ||
|
|
||
| Add Vite middleware to handle 404s in workflow routes from Nitro and silence undefined unhandled rejections |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/sveltekit": patch | ||
| --- | ||
|
|
||
| Fix SvelteKit plugin reading deleted files on HMR |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@workflow/example-sveltekit": patch | ||
| --- | ||
|
|
||
| Fix Sveltekit workbench workflows directory location | ||
|
|
||
| Move workflows from root to src/workflows for proper Sveltekit bundling and deployment |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,10 +63,21 @@ export class LocalBuilder extends BaseBuilder { | |
| inputFiles, | ||
| }); | ||
|
|
||
| const webhookRouteFile = join(this.#outDir, 'webhook.mjs'); | ||
|
|
||
| await this.createWebhookBundle({ | ||
| outfile: join(this.#outDir, 'webhook.mjs'), | ||
| outfile: webhookRouteFile, | ||
| bundle: false, | ||
| }); | ||
|
|
||
| // Post-process the generated file to wrap with SvelteKit request converter | ||
|
Collaborator
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. This comment seems to be wrong. doesn't look like we're doing anything with the svekltekit request converter... also were we not already doing this process.on injection elsewhere? does nitro really need to do this for every framework? seems like some frameworks didn't have this issue and already implemented it internally
Collaborator
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. ah I just say @pi0's comment below. seems like we should get rid of this if the error was elsewhere no?
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. ah yeah, it was copy pasted from the sveltekit one. i was running into errors with hono i believe throwing unhandled rejections. will run a ci test to confirm this
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. ah nvm it was vite, just reverted and it happens |
||
| let webhookRouteContent = await readFile(webhookRouteFile, 'utf-8'); | ||
|
Collaborator
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. Nitro based builds doesn't need it (nitro already traps errors, i think it should be behind a flag or something for builder)
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. Hmmmm okay that's interesting because I was running into errors for vite or hono I believe for this^
Collaborator
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. I agree that this should probably go into the builder code itself and happy to merge this into my PR still and work on this upstream to simplify
Collaborator
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. and then if only vite needs it as you're saying, then the vite plugin can specifically wire that flag in when using nitro |
||
|
|
||
| // NOTE: This is a workaround to avoid crashing in local dev when context isn't set for waitUntil() | ||
| webhookRouteContent = `process.on('unhandledRejection', (reason) => { if (reason !== undefined) console.error('Unhandled rejection detected', reason); }); | ||
| ${webhookRouteContent}`; | ||
|
|
||
| await writeFile(webhookRouteFile, webhookRouteContent); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,13 @@ | ||
| import type { Nitro } from 'nitro/types'; | ||
| import type { Plugin } from 'vite'; | ||
| import type { HotUpdateOptions, Plugin } from 'vite'; | ||
| import { LocalBuilder } from './builders.js'; | ||
| import type { ModuleOptions } from './index.js'; | ||
| import nitroModule from './index.js'; | ||
| import { workflowRollupPlugin } from './rollup.js'; | ||
|
|
||
| export function workflow(options?: ModuleOptions): Plugin[] { | ||
| let builder: LocalBuilder | undefined; | ||
|
|
||
| return [ | ||
| workflowRollupPlugin(), | ||
| { | ||
|
|
@@ -18,9 +21,93 @@ export function workflow(options?: ModuleOptions): Plugin[] { | |
| ...options, | ||
| _vite: true, | ||
| }; | ||
| if (nitro.options.dev) { | ||
| builder = new LocalBuilder(nitro); | ||
| } | ||
| return nitroModule.setup(nitro); | ||
| }, | ||
| }, | ||
| // NOTE: This is a workaround because Nitro passes the 404 requests to the dev server to handle. | ||
| // For workflow routes, we override to send an empty body to prevent Hono/Vite's SPA fallback. | ||
| configureServer(server) { | ||
| // Add middleware to intercept 404s on workflow routes before Vite's SPA fallback | ||
| return () => { | ||
| server.middlewares.use((req, res, next) => { | ||
| // Only handle workflow webhook routes | ||
| if (!req.url?.startsWith('/.well-known/workflow/v1/')) { | ||
| return next(); | ||
| } | ||
|
|
||
| // Wrap writeHead to ensure we send empty body for 404s | ||
| const originalWriteHead = res.writeHead; | ||
| res.writeHead = function (this: typeof res, ...args: any[]) { | ||
| const statusCode = typeof args[0] === 'number' ? args[0] : 200; | ||
|
|
||
| // For 404s on workflow routes, ensure we're sending the right headers | ||
| if (statusCode === 404) { | ||
| // Set content-length to 0 to prevent Vite from overriding | ||
| res.setHeader('Content-Length', '0'); | ||
adriandlam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // @ts-expect-error - Complex overload signature | ||
| return originalWriteHead.apply(this, args); | ||
| } as any; | ||
|
|
||
| next(); | ||
| }); | ||
| }; | ||
| }, | ||
| // TODO: Move this to @workflow/vite or something since this is vite specific | ||
|
Collaborator
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. You can start this by extracting it to a new function. Vite plugin can return an array (so reusable parts can be shared between nitro/svelete) |
||
| async hotUpdate(options: HotUpdateOptions) { | ||
| const { file, server, read } = options; | ||
|
|
||
| // Check if this is a TS/JS file that might contain workflow directives | ||
| const jsTsRegex = /\.(ts|tsx|js|jsx|mjs|cjs)$/; | ||
| if (!jsTsRegex.test(file)) { | ||
| return; | ||
| } | ||
|
|
||
| // Read the file to check for workflow/step directives | ||
| let content: string; | ||
| try { | ||
| content = await read(); | ||
| } catch { | ||
| // File might have been deleted - trigger rebuild to update generated routes | ||
| console.log('Workflow file deleted, rebuilding...'); | ||
| if (builder) { | ||
| await builder.build(); | ||
|
Contributor
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. The hotUpdate handler in the Nitro Vite plugin is missing error handling around View Details📝 Patch Detailsdiff --git a/packages/nitro/src/vite.ts b/packages/nitro/src/vite.ts
index f9fc187..c6b9705 100644
--- a/packages/nitro/src/vite.ts
+++ b/packages/nitro/src/vite.ts
@@ -47,7 +47,13 @@ export function workflow(options?: ModuleOptions): Plugin[] {
// File might have been deleted - trigger rebuild to update generated routes
if (builder) {
console.log('Workflow file deleted, rebuilding...');
- await builder.build();
+ try {
+ await builder.build();
+ } catch (buildError) {
+ // Build might fail if files are being deleted during test cleanup
+ // Log but don't crash - the next successful change will trigger a rebuild
+ console.error('Build failed during file deletion:', buildError);
+ }
}
server.ws.send({
type: 'full-reload',
@@ -69,7 +75,14 @@ export function workflow(options?: ModuleOptions): Plugin[] {
// Manually trigger workflow rebuild
if (builder) {
console.log('Workflow file changed, rebuilding...');
- await builder.build();
+ try {
+ await builder.build();
+ } catch (buildError) {
+ // Build might fail if files are being modified/deleted during test cleanup
+ // Log but don't crash - the next successful change will trigger a rebuild
+ console.error('Build failed during HMR:', buildError);
+ return;
+ }
}
// Trigger full reload to refresh the browser
AnalysisMissing error handling in hotUpdate handler crashes dev server on build failuresWhat fails: The How to reproduce:
Result: Unhandled promise rejection in HMR handler. Dev server becomes unstable. Expected: Build errors should be logged but not crash the HMR handler, allowing the dev server to remain stable. The next successful file change should trigger another rebuild. Evidence: The identical |
||
| } | ||
| server.ws.send({ | ||
| type: 'full-reload', | ||
| path: '*', | ||
|
Collaborator
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. hmm too aggressive?
Collaborator
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. probably fine for now and we can revisit |
||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const useWorkflowPattern = /^\s*(['"])use workflow\1;?\s*$/m; | ||
| const useStepPattern = /^\s*(['"])use step\1;?\s*$/m; | ||
|
|
||
| if ( | ||
| !useWorkflowPattern.test(content) && | ||
| !useStepPattern.test(content) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Trigger full reload - this will cause Nitro's dev:reload hook to fire, | ||
| // which will rebuild workflows and update routes | ||
| console.log('Workflow file changed, rebuilding...'); | ||
| if (builder) { | ||
| await builder.build(); | ||
| } | ||
| server.ws.send({ | ||
| type: 'full-reload', | ||
| path: '*', | ||
| }); | ||
|
|
||
| // Let Vite handle the normal HMR for the changed file | ||
| return; | ||
| }, | ||
| }, | ||
| ]; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| import { Hono } from 'hono'; | ||
| import { getHookByToken, getRun, resumeHook, start } from 'workflow/api'; | ||
| import { hydrateWorkflowArguments } from 'workflow/internal/serialization'; | ||
| import { allWorkflows } from './_workflows.js'; | ||
| import { | ||
| WorkflowRunFailedError, | ||
| WorkflowRunNotCompletedError, | ||
| } from 'workflow/internal/errors'; | ||
| import { hydrateWorkflowArguments } from 'workflow/internal/serialization'; | ||
| import { allWorkflows } from './_workflows.js'; | ||
|
|
||
| const app = new Hono(); | ||
|
|
||
|
|
@@ -163,8 +163,8 @@ app.post('/api/hook', async ({ req }) => { | |
| } catch (error) { | ||
| console.log('error during getHookByToken', error); | ||
| // TODO: `WorkflowAPIError` is not exported, so for now | ||
|
Collaborator
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. cc @TooTallNate we should have a |
||
| // we'll return 404 assuming it's the "invalid" token test case | ||
| return Response.json(null, { status: 404 }); | ||
| // we'll return 422 assuming it's the "invalid" token test case | ||
adriandlam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return Response.json(null, { status: 422 }); | ||
| } | ||
|
|
||
| await resumeHook(hook.token, { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // NOTE: This route isn't needed/ever used, we're just | ||
| // using it because webpack relies on esbuild's tree shaking | ||
|
|
||
| import { start } from 'workflow/api'; | ||
| import { addTenWorkflow } from '@/workflows/98_duplicate_case'; | ||
|
|
||
| export async function GET(_: Request) { | ||
| const run = await start(addTenWorkflow, [10]); | ||
| const result = await run.returnValue; | ||
| return Response.json({ result }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| #!/bin/bash | ||
adriandlam marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| set -e | ||
|
|
||
| # Script to recursively resolve all symlinks in the app directory | ||
| # This is needed for CI where Next.js dev mode doesn't work well with symlinks | ||
|
|
||
| # Only run in CI | ||
| if [ -z "$CI" ]; then | ||
| echo "Error: This script should only be run in CI environments" | ||
| echo "If you need to resolve symlinks locally, run it manually with CI=true" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Resolving all symlinks in current directory..." | ||
|
|
||
| # Find all symlinks in current directory (including nested ones), excluding gitignored files | ||
| git ls-files -z --cached --others --exclude-standard | xargs -0 -I {} sh -c 'test -L "{}" && echo "{}"' | while read -r symlink; do | ||
| # Get the target of the symlink | ||
| target=$(readlink "$symlink") | ||
|
|
||
| # Check if target is absolute or relative | ||
| if [[ "$target" = /* ]]; then | ||
| resolved_target="$target" | ||
| else | ||
| # Resolve relative symlink path | ||
| symlink_dir=$(dirname "$symlink") | ||
| resolved_target="$symlink_dir/$target" | ||
| fi | ||
|
|
||
| echo "Resolving: $symlink -> $resolved_target" | ||
|
|
||
| # Remove the symlink | ||
| rm "$symlink" | ||
|
|
||
| # Copy the target to the symlink location | ||
| if [ -d "$resolved_target" ]; then | ||
| cp -r "$resolved_target" "$symlink" | ||
| else | ||
| cp "$resolved_target" "$symlink" | ||
| fi | ||
| done | ||
|
|
||
| echo "All symlinks resolved successfully!" | ||
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.
crazy
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.
cc @pi0 you know about this?