feat(next): Allow specifying workflow directories#1159
feat(next): Allow specifying workflow directories#1159rovo89 wants to merge 1 commit intovercel:mainfrom
Conversation
This brings back the option introduced in vercel#843 (and removed in vercel#961), but with a slightly different meaning. Instead of specifying directories of pages whiuch call server actions which trigger workflows, the option is now meant to directly specify the directory with the workflow files. That removes two layers of indirection, imports way less files and therefore really improves performance as the original PR intended (~15s -> ~2.5s on a medium-sized app).
🦋 Changeset detectedLatest commit: 7e4c85b The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@rovo89 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
ijjk
left a comment
There was a problem hiding this comment.
Hi have you tried upgrading to latest workflow package and Next.js >= v16.2.0-canary.48 and then enabling this flag in workflows:
That will no longer use esbuild for bundling steps and discovery workflows/steps so should see the perf improvement without having to worry about a config like this.
I have just tried it. Without any other changes, I get: The "Run Created", "Run Started" and "Step Created" events exist, so it seems to have found the workflow, but not the step. Works find with |
|
Hi, can you share exact Next.js version did you use latest canary Also same results The lazy discovery is collecting the workflow/step usage in the workflow's loader then generating the route entries for webpack or turbopack to handle from that point. It's goal is to remove using esbuild to pass over all entries which is very expensive. |
| export default withWorkflow(nextConfig, { | ||
| workflows: { | ||
| dirs: ['workflows', 'src/workflows'], // [!code highlight] | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I don't think we should do dirs (for other reasons - like @ijjk mentioned - the lazy approach is the correct answer here).
but if we were to do dirs or add new configs, we shouldn't be putting it in a enw config options at the end of withWorkflow I think. instead the signature of withWorkflow should become withWorkflow(NextConfig & { workflow: WorkflowConfig }) imo (including the local port/datadir options and future config options)
There was a problem hiding this comment.
If the lazy approach works, then sure, I'll use it. Just began testing it again.
But what would the call to withWorkflow look like then? Keep in mind that the config might be a function, so we can't easily pass {...nextConfig, workflow: {...}}. Putting the option in the "raw" object literal would require typing it as NextConfigOrFn & WorkflowConfig, which again wouldn't work for functions and would be an extra step. I don't see what's wrong with the current signature, withSentryConfig does it the same way. Maybe Next.js could give an easier way for multiple modules to add config, but then you'd also need to care about namespaces, ...
| dirs: ['.'], // Different apps that use nitro have different directories | ||
| }), | ||
| buildTarget: 'next', // Placeholder, not actually used | ||
| buildTarget: 'standalone', // Placeholder, not actually used |
There was a problem hiding this comment.
'standalone; means something else. this could be nitro but honestly better to leave it out of this PR since it's not used anyway
There was a problem hiding this comment.
unless this was needed for some reason?
There was a problem hiding this comment.
It complained about the missing dirsAreEntrypoints, but yeah, could just pass that.
|
|
||
| **Solution:** Use the `workflows.dirs` option to point directly at the directories containing your workflow files. By default, `withWorkflow` uses entrypoint directories (`pages`, `app`, `src/pages`, `src/app`) as starting points and traces their imports to discover workflows, which in large applications can involve crawling thousands of files. | ||
|
|
||
| If your workflows live in a dedicated directory, configure `dirs` to only scan that directory: |
There was a problem hiding this comment.
the other problem with "dirs" is that it causes split brain I think:
the client mode/webpack transformation still has to happen in the actual code (api routes/server actions) that trigger workflows. we can't skip that - and that code won't live in the workflows directory
dirs feels like it misleads you into believing you've limited the search space to specific directory when you're only limiting the search space of actual workflows/steps imo
I just tried it again,
Both. Do I have to enable any other config? |
|
Any chance you could share the repo/a repro? Our E2E tests are currently running against this setup with Next.js canary so would love to track down where the setup is failing. |
|
I can also confirm that |
|
@ijjk Here's a very simple repro: https://github.com/rovo89/lazy-workflow. Minimal changes compared to |
|
I also tried moving the directives into the functions themselves, doesn't help. I had switched to the file-level directives because Prettier kept putting parentheses around them, and it's a bit shorter. Moving them both functions into one file (rovo89/lazy-workflow@bbd5076) solves it, but the recommendation is to keep them separate: https://useworkflow.dev/docs/foundations/workflows-and-steps#project-structure Webpack also doesn't work. |
|
@rovo89 thanks for sharing that context, I'll take a closer look this not working as expected with webpack is surprising so there's probably a higher level bug here with the lazy setup we can address |
This brings back the option introduced in #843 (and removed in #961), but with a slightly different meaning. Instead of specifying directories of pages whiuch call server actions which trigger workflows, the option is now meant to directly specify the directory with the workflow files. That removes two layers of indirection, imports way less files and therefore really improves performance as the original PR intended (~15s -> ~2.5s on a medium-sized app).