temporalio / sdk-typescript

Temporal TypeScript SDK
Other
528 stars 104 forks source link

Remove workflowsPath in favor or imports (like activities) #1540

Open marc-wilson opened 3 weeks ago

marc-wilson commented 3 weeks ago

Is your feature request related to a problem? Please describe.

We use an nx monorepo for our code base and we are trying to create our first workflows. Since most of our libraries/packages are written in Nestjs, we want our workers to be nestjs so that we can leverage them without having to re-write a bunch of dependencies.

However, since NestJs uses webpack by default, you end up with a single main.js file after build and your workflowsPath won't resolve.

Describe the solution you'd like

Instead of providing a file path where your workflows exist, we should be able to use native imports and register our workflows that way. Much like we do with activities.

ie:

import * as activities from './app/activities';
const worker = await Worker.create({
    connection,
    namespace: 'default',
    taskQueue: 'hello-world',
    // Workflows are registered using a path as they run in a separate JS context.
    workflowsPath: require.resolve('./app/workflows'),
    activities, // <-- from the imports
  });

The ideal solution would be something like this:

import * as activities from './app/activities';
import * as workflows from './app/workflows';
const worker = await Worker.create({
    connection,
    namespace: 'default',
    taskQueue: 'hello-world',
    workflows,
    activities,
  });

Additional context

It seems many others have hit this and all of the workarounds are pretty unfavorable as it comes down to getting off webpack (which comes with other issues) or hacking webpack with hard-coded outputs and entry points.

According to the temporal docs, there is another applicable property called workflowBundleConfig but it's not clear how it's used.

I am sure this question has been asked multiple times since there are plenty of hits in temporal community threads. However, there isn't any great solution out there. Obviously, there's a finger-pointing game to play since a valid question may be, "why are you using webpack for backend code?" Well, the answer is we don't have much of a choice with NX and Nest. If you remove NX/Nest from the equation, webpack goes with it and problem is solved. But, we are left with a bunch of work to re-write packages.

Before throwing in the towel on NX/Nest, I would like to understand why things are setup like this instead of following the same pattern as the activities import.

If there is a non-hacky example of NX and Temporal being used to together that supposrts importing dependencies and the usage of decorators, I would love to see it. The ones that are currently out there leverage a hacked-together esbuild or some other heavy-config based implementation.

mjameswh commented 3 weeks ago

That's indeed a recurring question. Believe me, we would definitely prefer passing Workflow functions or objects directly to the Worker; that would be much easier for everyone, including us.

But that's simply not possible.

Why? The short answer is already in your code snippet: "Workflows are registered using a path as they run in a separate JS context". That "separate JS context" is a Node.js VM, itself running in a Node.js Worker Thread.

Both of these layers (VM and Worker Thread) have their own requirements that point toward the necessity of having a distinct Workflow Bundle.

But then… Why do we even need these two isolation layers? Because Workflow code needs to be deterministic. Among other things, the Sandbox allows us to mock various global functions to ensure that they behave deterministically (e.g. Date.now(), Math.random(), setTimeout()), and to control execution order of microtasks (Promise and await). These are intrusive changes to the JS execution environment, which would be unsafe to do on the main Node.js thread.

Activities don't need to go through that type of bundling because they don't have determinism requirement.

Note that I'm intentionally cutting down on details here. There would be much more to say, but time simply doesn't permit.