Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
workflow apis tweak.md

now that we have a cohesive look at workflow APIs, some issues remain:

  • createActivityHandle isnt really a handle, in the way that createWorkflowHandle and createChildWorkflowHandle are. it returns a function that you call.
    • users are confused by the proxy destructure which a very fancy way of doing type safety and ensuring consistent naming
  • defineSignal/Query dont add much value since they just create an object
    • extra setListener api that is doing the real work, basically 2 different functions branching by def.type

just taking another crack at api design.

simplified query and signal api

status quo

export const unblockSignal = wf.defineSignal('unblock');
export const isBlockedQuery = wf.defineQuery('isBlocked');

export async function unblockOrCancel() {
  let isBlocked = true;
  wf.setListener(unblockSignal, () => void (isBlocked = false));
  wf.setListener(isBlockedQuery, () => isBlocked);
  console.log('Blocked');
  try {
    await wf.condition(() => !isBlocked);
    console.log('Unblocked');
  }
  catch (err) {
    if (err instanceof wf.CancelledFailure) {
      console.log('Cancelled');
    }
    throw err;
  }
}

proposal

if we eliminate setListener...

// using exportable definitions
export const onSetState = useSignal<boolean>('setState');
export const getIsBlocked = useQuery<boolean>('isBlocked');

export async function unblockOrCancel() {
  let isBlocked = true;
  onSetState((newState) => void (isBlocked = newState)); // boolean
  getIsBlocked(() => isBlocked);
  // ...
}

if they dont like on (because of the subscribe implication) they can name it setStateHandler or BlockedQueryResolver or whatever they like... we dont prescribe the naming.

for those who

  • don't need to export the signaldef/querydefs for strong typing for invocation (eg in the nextjs example its pretty inconvenient to import the types from the temporal folder into the nextjs folder, most people wont even bother)
  • don't need to reassign the listener

this enables inlining and reduces naming need:

// using strings
export async function unblockOrCancel() {
  let isBlocked = true;
  useSignal('unblock', () => void (isBlocked = false));
  useQuery('isBlocked', () => isBlocked);
  // ...
}

renamed Activity api

status quo

import { createActivityHandle } from '@temporalio/workflow';
import type * as activities from './activities';

const { greet } = createActivityHandle<typeof activities>({
  startToCloseTimeout: '1 minute',
});

/** A workflow that simply calls an activity */
export async function example(name) {
  return await greet(name);
}
  • this is decent tbh, but for some people (who would like to use typescript, but are not typescript gods), <typeof activities> is a foreign language and hard to decipher.
  • i am worried that people will just copy paste this and not really intuitively understand how to manipulate activities to suit their code style.
  • it also requires people to barrel all types into a single activities file (not really, but people will treat it that way)... would be nice to let people componentize or combine as they wish

proposal

i'd like to make clear that this is "just" a function and that we are importing from worker that must have this activity registered.

import { useActivity } from '@temporalio/workflow';
import type greet from './activities/greet'; // very clear that barrel file is optional

const invokeGreet = useActivity<greet>('greet', {
  startToCloseTimeout: '1 minute',
  // retries, etc
});

/** A workflow that simply calls an activity */
export async function example(name) {
  return await invokeGreet('world')
}

this means that you cant do the fancy multiple destructures, but hopefully usage will be much simplier because "less magic"...

import { useActivity, ActivityOptions } from '@temporalio/workflow';
import type foo, bar, baz from './activities'

const options: ActivityOptions =  {
  startToCloseTimeout: '1 minute',
  // retries, etc
}
const invokeFoo = useActivity<foo>('foo', options)
const invokeBar = useActivity<bar>('bar', options)
const invokeBaz = useActivity<baz>('baz', options)

/** A workflow that simply calls an activity */
export async function example(name) {
  await invokeFoo('world')
  await invokeBar('world')
  await invokeBaz(123, 345)
}
@bergundy
Copy link

Signals and Queries

For the first point, I think we clarified in the proposal why we want the definitions and why the definitions do not have a method to attach a listener.

After looking at Joe's pendulum code I think we can make a more JS friendly API if we break setListener into setQueryListener and setSignalListener.

Both of the proposed methods will accept a string as well as a definition.

Example for queries:

const gameInfoQuery = defineQuery<GameInfo>('getGameInfo');

export async function pendulum(info: GameInfo): Promise<void> {
  // ...
  setQueryListener(gameInfoQuery, () => gameInfo);
  // or
  setQueryListener('getGameInfo', () => gameInfo);
}

Activity Handles

As for the second point.
I've been thinking about this too.
There are a few drawbacks to how we create activity handles and how they are used.
a) Mixing the types with the handle creation is a bit confusing, and it could be simplified.
b) Passing an activityId to createActivityHandle will cause all activities returned from that call to share the same activityId.
c) In the future we will probably support signaling and activities from workflows, the current API will not allow this.

An alternative proposal is to split the activity creation and type inference helpers.

For TS users:

import * as wf from '@temporalio/workflow';
import types * as activityTypes from './activities';
const { greet } = wf.activityTypeProxy<activityTypes>();

export async function myWorkflow(): Promise<void>() {
  const handle = wf.createActivityHandle(greet, { startToCloseTimeout: '1m' });
  // OR
  const handle = wf.createActivityHandle<typeof activities.greet>('greet', { startToCloseTimeout: '1m' });
  await handle.start(arg1, arg2);
  await myActivity.signal(someFutureSpecActivityDefinition, arg1, arg2);
  await handle.result();
  await handle.execute(arg1, arg2); // error: Activity has already been started
}

For JS users:

import * as wf from '@temporalio/workflow';

export async function myWorkflow() {
  const handle = wf.createActivityHandle('greet', { startToCloseTimeout: '1m' });
  await handle.start(arg1, arg2);
  await myActivity.signal(someFutureSpecActivityDefinition, arg1, arg2);
  await handle.result();
  await handle.execute(arg1, arg2); // error: Activity has already been started
}

Notes

  • activityTypeProxy makes up for the fact that activity implementations cannot be imported directly from the workflow and let's the user infer both activity name and type.
  • The start / result / signal methods will not be implemented in the first step, they can be added later when activities support signaling.

@lorensr
Copy link

lorensr commented Oct 18, 2021

const gameInfoQuery = defineQuery<GameInfo>('getGameInfo');

export async function pendulum(info: GameInfo): Promise<void> {
  // ...
  setQueryListener(gameInfoQuery, () => gameInfo);
  // or
  setQueryListener('getGameInfo', () => gameInfo);
}

The second case seems to address Shawn's concern, since defineQuery is avoided, but the typed/recommended case makes it a little worse, adding another "Query" to retype (3 -> 4).

I forget why we chose "Listener", but the term "Handler" feels like a better fit to me:

  setQueryHandler('getGameInfo', () => gameInfo);

Activities

Types

import types * as activityTypes from './activities';
const { greet } = wf.activityTypeProxy<activityTypes>();

Pros: removes <typeof activities>
Cons: adds another step; still has types * as activityTypes from './activities'

Handle

import type * as activities from './activities';

const { greet } = useActivities<typeof activities>({
  startToCloseTimeout: '1 minute',
});

or

import types * as activityTypes from './activities';
const { greet } = wf.activityTypeProxy<activityTypes>();
const greetFn = useActivity(greet, { startToCloseTimeout: '1m' });

@bergundy
Copy link

We can't get rid of typeof activities unfortunately.
image

I forget why we chose "Listener", but the term "Handler" feels like a better fit to me:
To avoid confusion with the handle concept.

After talking to Maxim, he mentioned that if activities probably won't become addressable and instead we'll add a new concept (like actors).

I still don't have a good solution for the first issue, another couple options that came to mind:

  • setListener also accepts an object param:
    setListener({ query: 'isBlocked' }, () => isBlocked);
  • setListener that accepts a string and effectively sets a listener for both a query and signal:
    setListener('unblock', () => void (isBlocked = false));
    setListener('isBlocked', () => isBlocked);

I like the following solution because it reduces the amount of "magic" in the API.

import types * as activities from './activities';
const { greet } = wf.activityTypeProxy<typeof activities>();
const greetFn = useActivity(greet, { startToCloseTimeout: '1m' });
// or the more verbose and error prone approach
const greetFn = useActivity<typeof activities.greet>('greet', { startToCloseTimeout: '1m' });
// JS users can use just a string
const greetFn = useActivity('greet', { startToCloseTimeout: '1m' });

@lorensr
Copy link

lorensr commented Oct 19, 2021

Signals & queries

FWIW, I think handle and handler nouns are clearly different English & coding concepts and still prefer setHandler 🤷‍♂️ Mainly because handling seems more accurate about what the function is doing than listening, particularly in the case of queries, but also because listeners sounds like I can set multiple listeners, and handler doesn't.

We need to know:

  • string name of the signal/query
  • the listener function
  • the type the listener returns (for recommended TS use)
  • whether signal or query (ideally, but not necessary, if we're okay setting listener for both signal and query of same name)

Current way of getting all that info without repetition:

setListener(defineQuery<GameInfo>('getGameInfo'), () => gameInfo);

Would this work? It reads nicer to me:

setQueryListener<GameInfo>('getGameInfo', () => gameInfo)

Activities

I like in this one that I can call the function greet:

// A
import types * as activities from './activities';

const greet = useActivity<typeof activities.greet>('greet', { startToCloseTimeout: '1m' });

The destructuring complexity is gone. The import type * as and typeof complexity remains. For those two things, are these the options?

// B
import type greet from './activities/greet';

const greetFn = useActivity<typeof greet>('greet', { startToCloseTimeout: '1m' });
// C
import { GreetType } from './activities/greet';
// or
import type { GreetType } from './activities/greet';

const greet = useActivity<GreetType>('greet', { startToCloseTimeout: '1m' });

With B I don't like greetFn, and with C I don't like having to define GreetType. So I prefer A and adding to the useActivity docs what the activities file import/export options are.

@sw-yx
Copy link
Author

sw-yx commented Oct 25, 2021

setHandler('unblock', () => void (isBlocked = false)); // effectively only a signal
setHandler('isBlocked', () => isBlocked); // signal AND query
// problems - some stuff is illegal in queries

// typesafe
const myQuery = defineQuery<[boolean]>('isBlocked') // name + type
setListener(myQuery, () => isBlocked) 


// loren typesafe
export type GameInfo = boolean
setQueryListener<GameInfo>('getGameInfo', () => gameInfo)

const myQuery = defineQuery<[boolean]>('isBlocked') // name + type
const mySignal = defineSignal<[boolean]>('isBlocked') // name + type

setHandler(mySignal, () => void (isBlocked = false));
setHandler(myQuery, () => isBlocked);



setHandler<someType>({ type: 'signal', name: 'plsBlock' }), () => void (isBlocked = false));
setHandler<someType>({ type: 'query', name: 'isBlocked' }), () => void (isBlocked = false));

@lorensr
Copy link

lorensr commented Oct 25, 2021

Sounded like we have two things to decide:

  1. Do we want to provide the single-activity option instead of or in addition to? (Only benefit is no destructuring)

Sounded like no?

  1. If multiple, what do we want to call it? I like useActivity or useActivities or both.
import type * as activities from './activities';

// single
const greet = useActivity<typeof activities.greet>('greet', { startToCloseTimeout: '1m' });
const salute = useActivity<typeof activities.salute>('salute', { startToCloseTimeout: '1m' });

// multiple "useActivities"
const { greet, salute } = useActivities<typeof activities>({
  startToCloseTimeout: '1 minute',
});

// multiple "createActivityFunctions"
const { greet, salute } = createActivityFunctions<typeof activities>({
  startToCloseTimeout: '1 minute',
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment