Skip to content
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

[Bug] Cannot use Temporal with NextJS 15 because workflowTypes come from function names #1615

Open
rclmenezes opened this issue Jan 28, 2025 · 9 comments
Labels
bug Something isn't working

Comments

@rclmenezes
Copy link

What are you really trying to do?

Use Temporal with NextJS 15 in production

Describe the bug

Temporal Typescript uses the function name as the workflow type. This causes issues when the code is minified.

When using NextJS, the function names get mangled by default. In NextJS <=14, you could set swcMinify: false in your next-config to get around this issue:
vercel/next.js#74332 (comment)

However, that is not an option in Next 15, which disabled swcMinify as an option. The only option there is to make a custom webpack config to disable optimization altogether.

Minimal Reproduction

The official example of nextjs-temporal breaks:
temporalio/nextjs-temporal-one-click-template#3

Environment/Versions

All of them

Additional context

I think Temporal Typescript's SDK should support minification, full stop. Users shouldn't have to make a magic webpack config to make things happen.

@rclmenezes rclmenezes added the bug Something isn't working label Jan 28, 2025
@rclmenezes
Copy link
Author

rclmenezes commented Jan 28, 2025

One hack around I tried: export your workflows separately as an object:

import * as workflows from "./workflows";

export { workflows };

And then you can import the object instead:

      await temporalClient.workflow.start(workflows.myWorkflow, {
        args: [],
        taskQueue: TEMPORAL_TASK_QUEUE,
        workflowId: `whatever`,
      });

EDIT: nope this doesn't work either.

@rclmenezes
Copy link
Author

rclmenezes commented Jan 30, 2025

Here's a solution that worked -- disabling minification on the server. Here's my next-js config:

export default {
  webpack: (config) => {
    if (config.name === "server") {
      // Disable minification for server builds to avoid temporal build errors
      config.optimization.minimize = false;
    }
    return config;
  },
};

It sucks that we have to do this. Using function names is not great in the long run. Maybe a decorator is a better long-term solution, or turning workflows into classes of some kind.

@mjameswh
Copy link
Contributor

I totally understand how this kind of things can be frustrating, and I do agree that we need to improve the overall developer experience for Temporal + Next.js users.

My plan is to eventually provide plugins to ease integration between Temporal TS SDK and other major frameworks. The present issue could very easily be resolved by such a plugin, e.g. by having the plugin apply specific AST transformations during the bundling process.

@mjameswh
Copy link
Contributor

Using function names is not great in the long run. Maybe a decorator is a better long-term solution, or turning workflows into classes of some kind.

Sorry, I strongly disagree on this.

The difficulties you describe are the result of conflicting DX optimizations made respectively by the Temporal SDK and the Next.js teams, which are both sensible defaults for our respective community.

  • On the Next.js side — There is technically no strong requirement for bundling, minimizing and optimizing the backend code, but the Next.js community recognized that most developers would appreciate the reduced deployment size, and therefore decided to enable those options by default, leaving the possibility for users to opt out of that feature by using a configuration hook.

  • On the Temporal side — A Workflow Type is really just a simple string. We however recognized that most of our developers would appreciate the type safety provided by passing the Workflow function object, and therefore chose to promote that approach in most of our samples; users may however opt out of that feature by passing raw strings rather than functions. Similarly, we chose to define Workflow implementations as publicly exported functions on a module because we recognized that most of our users would appreciate the simplicity of that model; users may however opt in to implement alternate mapping logic (e.g. Workflows as classes or methods) by registering a single default Workflow function.

Default behaviors on both sides are sensible and in the best interest of our respective communities. They only conflict when trying to combine both libraries' defaults in a same project, at which point one needs to do some extra work or to simply opt out to some feature in either libraries.

Would it make sense to say that "Next.js stripping the name property on functions on backend code is not great in the long run"? Obviously not. That decision makes sense for most users in the Next.js community.

Similarly, the patterns we encourage regarding Workflow definition make sense for most users of the Temporal TypeScript SDK. A user may easily implement alternate approaches, including decorator functions and classes. But those alternatives also have drawbacks, and would definitely not be in the interest of most users.

@rclmenezes
Copy link
Author

rclmenezes commented Jan 31, 2025

Thanks for the in-depth response, James.

Framing it as "sensible defaults" on both sides make sense. That said -- I'm surprised that I'm the first to bring this up! I would have thought that most other batteries-included frameworks like Remix minimize backend code by default as well, but if I'm the first to bring this up, then I might be wrong!

Providing a NextJS plugin as a paved path could be a very neat approach. Or at least making the sample repo work in production :)

As an aside... I never knew that client.workflow.start takes a raw string as well as a function! That's a really great escape hatch. I wish I saw that in the docs. It opens the door to a lot of custom solutions as you suggest.

I still think that an optional decorator could be a great approach where we keep existing defaults and provide a nice paved path:

import {temporalWorkflow} from "temporalio/workflow";

@temporalWorkflow(workflowType='myWorkflow')
function myWorkflow() {
    ...
}

In this scenario, workflow.start could default back to the function name if some special attribute like fn.workflowName isn't present

For now, if I want minification, I'll probably make a quick and dirty custom wrapper for client.workflow.start:

const workflows: Record<string, WorkflowFn> = {};

async function startFlow<Name extends keyof typeof workflows>(name: Name, args: Parameters<typeof workflows[Name]>[0], workflowId: string) {
  const client = await getTemporalClient();
  return client.workflow.start(name, {
    args: [args],
    taskQueue: TEMPORAL_TASK_QUEUE,
    workflowId,
  });
}

Thanks again

@mjameswh
Copy link
Contributor

I still think that an optional decorator could be a great approach where we keep existing defaults and provide a nice paved path:

import {temporalWorkflow} from "temporalio/workflow";

@temporalWorkflow(workflowType='myWorkflow')
function myWorkflow() {
    ...
}

Yeah, we would have done that a very long time ago if that was possible, but unfortunately, decorators (both the ECMAScript Stage 3 proposal and the TypeScript's current spec) only support classes/methods/props… Not simple functions.

@mjameswh
Copy link
Contributor

For now, if I want minification, I'll probably make a quick and dirty custom wrapper for client.workflow.start:

const workflows: Record<string, WorkflowFn> = {};

async function startFlow<Name extends keyof typeof workflows>(name: Name, args: Parameters<typeof workflows[Name]>[0], workflowId: string) {
  const client = await getTemporalClient();
  return client.workflow.start(name, {
    args: [args],
    taskQueue: TEMPORAL_TASK_QUEUE,
    workflowId,
  });
}

Yeah, that would work.

Another possibility is to make a proxy object that has the type of your workflow.ts module, but simply return the property name as a string. As a proof of concept, I was able to get the nextjs-ecommerce-oneclick sample running in prod mode by adding this at the top of the startBuy.js file:

/**
 * @type {import('../../temporal/lib/workflows.js')}
 */
const workflows = new Proxy({}, {
  get: (target, prop) => {
    return prop;
  }
})

@mjameswh
Copy link
Contributor

mjameswh commented Jan 31, 2025

I would have thought that most other batteries-included frameworks like Remix minimize backend code by default as well

Nowadays, yes, but that's kind of a new thing. Back a few years ago, bundlers were all about frontend stuff. On the server side, you'd always have a node_modules directory.

I'd say that IMHO, a major contribution to that trend is that most frameworks are now backed by companies that want to sell you hosting services for your app, so they design the tooling to be very efficient to host, even if that means being less convenient for the developers. And once developers know that they could minimize their backend code, they feel like they are missing something if they don't.

This has a few drawbacks, notably with libraries that have native dependencies, that load files dynamically at runtime (e.g. plugins, languages files, etc), and those that depends on preserving exact properties names and objects names (including Temporal, of course, but also Protobuf, ORMs, and others). That's why most of those frameworks maintain some internal list of libraries known to be non-minimizable safe (see Next.js), but in this particular case, it is your code, and not Temporal's NPM packages, that need special handling, so that list can't help.

@rclmenezes
Copy link
Author

decorators (both the ECMAScript Stage 3 proposal and the TypeScript's current spec) only support classes/methods/props… Not simple functions.

Ugh, I didn't know that. I honestly never use decorators in TS, they're a little too Python-y for me.

Another possibility is to make a proxy object that has the type of your workflow.ts module, but simply return the property name as a string.

I like it -- thanks!

For now, I'm probably just going to disable minification and call it a day. I'm hosting on ECS anyway.

I'd say that IMHO, a major contribution to that trend is that most frameworks are now backed by companies that want to sell you hosting services for your app, so they design the tooling to be very efficient to host, even if that means being less convenient for the developers.

You're 100% right about this. My favorite example is actually Next.js middleware. It's so custom built for their own edge infrastructure that it's practically useless.

Literally every other framework I've used in my career has middleware that sees both request and response so you can log or have custom error class handling or identify users in Sentry or whatever. Instead, I have to make a custom wrapApiHandler fo things like this... or move my API to a separate service entirely and deal with the extra infra. What a mess!

Anyway, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants