fix: Better error messages, and more (#1126)

- Better error message when `executeWorkflow()` and similar are given a
workflowFunction fucntion that is anonymous
- Better error message when Workflow-only APIs are called from
non-Workflow execution context
- On `create`, do not use `@tsconfig/node20`
- Include proto comments in API files generated by protobufjs
- Minor refactor of schedule's error handling

Author: mjameswh

packages/client/src/schedule-client.ts

@@ -260,7 +260,6 @@ export class ScheduleClient extends BaseClient {
       const res = await this.workflowService.createSchedule(req);
       return { conflictToken: res.conflictToken };
     } catch (err: any) {
-      if (err instanceof TypeError) throw err;
       if (err.code === grpcStatus.ALREADY_EXISTS) {
         throw new ScheduleAlreadyRunning('Schedule already exists and is running', opts.scheduleId);
       }
@@ -292,21 +291,21 @@ export class ScheduleClient extends BaseClient {
     opts: CompiledScheduleUpdateOptions,
     header: Headers
   ): Promise<temporal.api.workflowservice.v1.IUpdateScheduleResponse> {
+    const req = {
+      namespace: this.options.namespace,
+      scheduleId,
+      schedule: {
+        spec: encodeScheduleSpec(opts.spec),
+        action: await encodeScheduleAction(this.dataConverter, opts.action, header),
+        policies: encodeSchedulePolicies(opts.policies),
+        state: encodeScheduleState(opts.state),
+      },
+      identity: this.options.identity,
+      requestId: uuid4(),
+    };
     try {
-      return await this.workflowService.updateSchedule({
-        namespace: this.options.namespace,
-        scheduleId,
-        schedule: {
-          spec: encodeScheduleSpec(opts.spec),
-          action: await encodeScheduleAction(this.dataConverter, opts.action, header),
-          policies: encodeSchedulePolicies(opts.policies),
-          state: encodeScheduleState(opts.state),
-        },
-        identity: this.options.identity,
-        requestId: uuid4(),
-      });
+      return await this.workflowService.updateSchedule(req);
     } catch (err: any) {
-      if (err instanceof TypeError) throw err;
       this.rethrowGrpcError(err, scheduleId, 'Failed to update schedule');
     }
   }
@@ -380,7 +379,7 @@ export class ScheduleClient extends BaseClient {
           scheduleId: raw.scheduleId,
 
           spec: decodeScheduleSpec(raw.info?.spec ?? {}),
-          action: raw.info?.workflowType?.name && {
+          action: raw.info?.workflowType && {
             type: 'startWorkflow',
             workflowType: raw.info.workflowType.name,
           },

packages/client/src/schedule-helpers.ts

@@ -2,6 +2,7 @@ import Long from 'long'; // eslint-disable-line import/no-named-as-default
 import {
   compileRetryPolicy,
   decompileRetryPolicy,
+  extractWorkflowType,
   LoadedDataConverter,
   mapFromPayloads,
   mapToPayloads,
@@ -208,7 +209,7 @@ export function decodeOverlapPolicy(input?: temporal.api.enums.v1.ScheduleOverla
 
 export function compileScheduleOptions(options: ScheduleOptions): CompiledScheduleOptions {
   const workflowTypeOrFunc = options.action.workflowType;
-  const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+  const workflowType = extractWorkflowType(workflowTypeOrFunc);
   return {
     ...options,
     action: {
@@ -222,7 +223,7 @@ export function compileScheduleOptions(options: ScheduleOptions): CompiledSchedu
 
 export function compileUpdatedScheduleOptions(options: ScheduleUpdateOptions): CompiledScheduleUpdateOptions {
   const workflowTypeOrFunc = options.action.workflowType;
-  const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+  const workflowType = extractWorkflowType(workflowTypeOrFunc);
   return {
     ...options,
     action: {

packages/client/src/workflow-client.ts

@@ -18,6 +18,7 @@ import {
   WorkflowExecutionAlreadyStartedError,
   WorkflowNotFoundError,
   WorkflowResultType,
+  extractWorkflowType,
 } from '@temporalio/common';
 import { composeInterceptors } from '@temporalio/common/lib/interceptors';
 import { History } from '@temporalio/common/lib/proto-utils';
@@ -345,7 +346,7 @@ export class WorkflowClient extends BaseClient {
     options: WithWorkflowArgs<T, WorkflowOptions>,
     interceptors: WorkflowClientInterceptor[]
   ): Promise<string> {
-    const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+    const workflowType = extractWorkflowType(workflowTypeOrFunc);
     assertRequiredWorkflowOptions(options);
     const compiledOptions = compileWorkflowOptions(ensureArgs(options));
 
@@ -369,7 +370,7 @@ export class WorkflowClient extends BaseClient {
     options: WithWorkflowArgs<T, WorkflowSignalWithStartOptions<SA>>,
     interceptors: WorkflowClientInterceptor[]
   ): Promise<string> {
-    const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+    const workflowType = extractWorkflowType(workflowTypeOrFunc);
     const { signal, signalArgs, ...rest } = options;
     assertRequiredWorkflowOptions(rest);
     const compiledOptions = compileWorkflowOptions(ensureArgs(rest));

packages/common/src/workflow-options.ts

@@ -154,3 +154,14 @@ export function compileWorkflowOptions<T extends CommonWorkflowOptions>(options:
     workflowTaskTimeout: msOptionalToTs(workflowTaskTimeout),
   };
 }
+
+export function extractWorkflowType<T extends Workflow>(workflowTypeOrFunc: string | T): string {
+  if (typeof workflowTypeOrFunc === 'string') return workflowTypeOrFunc as string;
+  if (typeof workflowTypeOrFunc === 'function') {
+    if (workflowTypeOrFunc?.name) return workflowTypeOrFunc.name;
+    throw new TypeError('Invalid workflow type: the workflow function is anonymous');
+  }
+  throw new TypeError(
+    `Invalid workflow type: expected either a string or a function, got '${typeof workflowTypeOrFunc}'`
+  );
+}

packages/create-project/src/helpers/install.ts

@@ -35,8 +35,14 @@ export async function updateNodeVersion({ root }: InstallArgs): Promise<void> {
   const versionAlreadyInPackageJson = 16;
   const minimumValidVersion = 14;
 
-  if (currentNodeVersion >= minimumValidVersion && currentNodeVersion !== versionAlreadyInPackageJson) {
-    const packageName = `@tsconfig/node${currentNodeVersion}`;
+  // The @tsconfig/node20 sets "--lib es2023", which require TypeScript 5.x.
+  // FIXME: Remove this once samples have been updated to TypeScript ^5.0.0.
+  const maximumValidVersion = 18;
+
+  const tsconfigNodeVersion = Math.max(minimumValidVersion, Math.min(currentNodeVersion, maximumValidVersion));
+
+  if (tsconfigNodeVersion !== versionAlreadyInPackageJson) {
+    const packageName = `@tsconfig/node${tsconfigNodeVersion}`;
 
     const packageExists = await isUrlOk(`https://registry.npmjs.org/${packageName}`);
     if (packageExists) {

packages/proto/scripts/compile-proto.js

@@ -1,4 +1,4 @@
-const { rm } = require('fs/promises');
+const { rm, readFile, writeFile } = require('fs/promises');
 const { resolve } = require('path');
 const { promisify } = require('util');
 const dedent = require('dedent');
@@ -42,6 +42,7 @@ async function compileProtos(dtsOutputFile, ...args) {
     'commonjs',
     '--force-long',
     '--no-verify',
+    '--alt-comment',
     '--root',
     '__temporal',
     resolve(require.resolve('protobufjs'), '../google/protobuf/descriptor.proto'),
@@ -59,6 +60,18 @@ async function compileProtos(dtsOutputFile, ...args) {
   console.log(`Creating protobuf TS definitions from ${coreProtoPath} and ${workflowServiceProtoPath}`);
   try {
     await promisify(pbjs.main)([...pbjsArgs, '--target', 'static-module', '--out', tempFile]);
+
+    // pbts internally calls jsdoc, which do strict validation of jsdoc tags.
+    // Unfortunately, some protobuf comment about cron syntax contains the
+    // "@every" shorthand at the begining of a line, making it appear as a
+    // (invalid) jsdoc tag. Similarly, docusaurus trips on <interval> and other
+    // things that looks like html tags. We fix both cases by rewriting these
+    // using markdown "inline code" syntax.
+    let tempFileContent = await readFile(tempFile, 'utf8');
+    tempFileContent = tempFileContent.replace(/(@(?:yearly|monthly|weekly|daily|hourly|every))/g, '`$1`');
+    tempFileContent = tempFileContent.replace(/<((?:interval|phase|timezone)(?: [^>]+)?)>/g, '`<$1>`');
+    await writeFile(tempFile, tempFileContent, 'utf-8');
+
     await promisify(pbts.main)(['--out', dtsOutputFile, tempFile]);
   } finally {
     await rm(tempFile);

packages/test/scripts/compile-proto.js

@@ -28,7 +28,17 @@ function mtime(path) {
 }
 
 async function compileProtos(outputFile, ...args) {
-  const pbjsArgs = [...args, '--wrap', 'commonjs', '--force-long', '--no-verify', '--out', outputFile, ...protoFiles];
+  const pbjsArgs = [
+    ...args,
+    '--wrap',
+    'commonjs',
+    '--force-long',
+    '--no-verify',
+    '--alt-comment',
+    '--out',
+    outputFile,
+    ...protoFiles,
+  ];
   return await promisify(pbjs.main)(pbjsArgs);
 }
 

packages/workflow/src/workflow.ts

@@ -2,6 +2,7 @@ import {
   ActivityFunction,
   ActivityOptions,
   compileRetryPolicy,
+  extractWorkflowType,
   IllegalStateError,
   LocalActivityOptions,
   mapToPayloads,
@@ -37,7 +38,7 @@ import {
   Handler,
   WorkflowInfo,
 } from './interfaces';
-import { LocalActivityDoBackoff, getActivator, maybeGetActivator } from './internals';
+import { Activator, LocalActivityDoBackoff, getActivator, maybeGetActivator } from './internals';
 import { LoggerSinks, Sinks } from './sinks';
 import { untrackPromise } from './stack-helpers';
 import { ChildWorkflowHandle, ExternalWorkflowHandle } from './workflow-handle';
@@ -108,7 +109,7 @@ function timerNextHandler(input: TimerInput) {
  * If given a negative number or 0, value will be set to 1.
  */
 export function sleep(ms: number | string): Promise<void> {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext('Workflow.sleep(...) may only be used from a Workflow Execution');
   const seq = activator.nextSeqs.timer++;
 
   const durationMs = Math.max(1, msToNumber(ms));
@@ -250,7 +251,9 @@ async function scheduleLocalActivityNextHandler({
  * @hidden
  */
 export function scheduleActivity<R>(activityType: string, args: any[], options: ActivityOptions): Promise<R> {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.scheduleActivity(...) may only be used from a Workflow Execution'
+  );
   if (options === undefined) {
     throw new TypeError('Got empty activity options');
   }
@@ -275,7 +278,9 @@ export async function scheduleLocalActivity<R>(
   args: any[],
   options: LocalActivityOptions
 ): Promise<R> {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.scheduleLocalActivity(...) may only be used from a Workflow Execution'
+  );
   if (options === undefined) {
     throw new TypeError('Got empty activity options');
   }
@@ -574,7 +579,9 @@ const CONDITION_0_PATCH = '__sdk_internal_patch_number:1';
  * It takes a Workflow ID and optional run ID.
  */
 export function getExternalWorkflowHandle(workflowId: string, runId?: string): ExternalWorkflowHandle {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.getExternalWorkflowHandle(...) may only be used from a Workflow Execution. Consider using Client.workflow.getHandle(...) instead.)'
+  );
   return {
     workflowId,
     runId,
@@ -696,9 +703,11 @@ export async function startChild<T extends Workflow>(
   workflowTypeOrFunc: string | T,
   options?: WithWorkflowArgs<T, ChildWorkflowOptions>
 ): Promise<ChildWorkflowHandle<T>> {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.startChild(...) may only be used from a Workflow Execution. Consider using Client.workflow.start(...) instead.)'
+  );
   const optionsWithDefaults = addDefaultWorkflowOptions(options ?? ({} as any));
-  const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+  const workflowType = extractWorkflowType(workflowTypeOrFunc);
   const execute = composeInterceptors(
     activator.interceptors.outbound,
     'startChildWorkflowExecution',
@@ -795,9 +804,11 @@ export async function executeChild<T extends Workflow>(
   workflowTypeOrFunc: string | T,
   options?: WithWorkflowArgs<T, ChildWorkflowOptions>
 ): Promise<WorkflowResultType<T>> {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.executeChild(...) may only be used from a Workflow Execution. Consider using Client.workflow.execute(...) instead.'
+  );
   const optionsWithDefaults = addDefaultWorkflowOptions(options ?? ({} as any));
-  const workflowType = typeof workflowTypeOrFunc === 'string' ? workflowTypeOrFunc : workflowTypeOrFunc.name;
+  const workflowType = extractWorkflowType(workflowTypeOrFunc);
   const execute = composeInterceptors(
     activator.interceptors.outbound,
     'startChildWorkflowExecution',
@@ -841,7 +852,8 @@ export async function executeChild<T extends Workflow>(
  * }
  */
 export function workflowInfo(): WorkflowInfo {
-  return getActivator().info;
+  const activator = assertInWorkflowContext('Workflow.workflowInfo(...) may only be used from a Workflow Execution.');
+  return activator.info;
 }
 
 /**
@@ -891,7 +903,9 @@ export function proxySinks<T extends Sinks>(): T {
           {
             get(_, fnName) {
               return (...args: any[]) => {
-                const activator = getActivator();
+                const activator = assertInWorkflowContext(
+                  'Proxied sinks functions may only be used from a Workflow Execution.'
+                );
                 activator.sinkCalls.push({
                   ifaceName: ifaceName as string,
                   fnName: fnName as string,
@@ -917,8 +931,10 @@ export function proxySinks<T extends Sinks>(): T {
 export function makeContinueAsNewFunc<F extends Workflow>(
   options?: ContinueAsNewOptions
 ): (...args: Parameters<F>) => Promise<never> {
-  const activator = getActivator();
-  const info = workflowInfo();
+  const activator = assertInWorkflowContext(
+    'Workflow.continueAsNew(...) and Workflow.makeContinueAsNewFunc(...) may only be used from a Workflow Execution.'
+  );
+  const info = activator.info;
   const { workflowType, taskQueue, ...rest } = options ?? {};
   const requiredOptions = {
     workflowType: workflowType ?? info.workflowType,
@@ -1041,7 +1057,9 @@ export function deprecatePatch(patchId: string): void {
 }
 
 function patchInternal(patchId: string, deprecated: boolean): boolean {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.patch(...) and Workflow.deprecatePatch may only be used from a Workflow Execution.'
+  );
   // Patch operation does not support interception at the moment, if it did,
   // this would be the place to start the interception chain
 
@@ -1075,6 +1093,7 @@ export function condition(fn: () => boolean, timeout: number | string): Promise<
 export function condition(fn: () => boolean): Promise<void>;
 
 export async function condition(fn: () => boolean, timeout?: number | string): Promise<void | boolean> {
+  assertInWorkflowContext('Workflow.condition(...) may only be used from a Workflow Execution.');
   // Prior to 1.5.0, `condition(fn, 0)` was treated as equivalent to `condition(fn, undefined)`
   if (timeout === 0 && !patched(CONDITION_0_PATCH)) {
     return conditionInner(fn);
@@ -1162,7 +1181,7 @@ export function setHandler<Ret, Args extends any[], T extends SignalDefinition<A
   def: T,
   handler: Handler<Ret, Args, T> | undefined
 ): void {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext('Workflow.setHandler(...) may only be used from a Workflow Execution.');
   if (def.type === 'signal') {
     if (typeof handler === 'function') {
       activator.signalHandlers.set(def.name, handler as any);
@@ -1195,7 +1214,9 @@ export function setHandler<Ret, Args extends any[], T extends SignalDefinition<A
  * @param handler a function that will handle signals for non-registered signal names, or `undefined` to unset the handler.
  */
 export function setDefaultSignalHandler(handler: DefaultSignalHandler | undefined): void {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.setDefaultSignalHandler(...) may only be used from a Workflow Execution.'
+  );
   if (typeof handler === 'function') {
     activator.defaultSignalHandler = handler;
     activator.dispatchBufferedSignals();
@@ -1236,7 +1257,9 @@ export function setDefaultSignalHandler(handler: DefaultSignalHandler | undefine
  * @param searchAttributes The Record to merge. Use a value of `[]` to clear a Search Attribute.
  */
 export function upsertSearchAttributes(searchAttributes: SearchAttributes): void {
-  const activator = getActivator();
+  const activator = assertInWorkflowContext(
+    'Workflow.upsertSearchAttributes(...) may only be used from a Workflow Execution.'
+  );
 
   const mergedSearchAttributes = { ...activator.info.searchAttributes, ...searchAttributes };
   if (!mergedSearchAttributes) {
@@ -1281,6 +1304,7 @@ export const log: LoggerSinks['defaultWorkerLogger'] = Object.fromEntries(
     return [
       level,
       (message: string, attrs: Record<string, unknown>) => {
+        assertInWorkflowContext('Workflow.log(...) may only be used from a Workflow Execution.)');
         return loggerSinks.defaultWorkerLogger[level](message, {
           // Inject the call time in nanosecond resolution as expected by the worker logger.
           [LogTimestamp]: getActivator().getTimeOfDay(),
@@ -1290,3 +1314,9 @@ export const log: LoggerSinks['defaultWorkerLogger'] = Object.fromEntries(
     ];
   })
 ) as any;
+
+function assertInWorkflowContext(message: string): Activator {
+  const activator = maybeGetActivator();
+  if (activator == null) throw new IllegalStateError(message);
+  return activator;
+}