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

Use name instead of id for instances in Workflows binding #2881

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ export const tests = {
{
// Test create instance
const instance = await env.workflow.create('foo', { bar: 'baz' });
assert.deepStrictEqual(instance.id, 'foo');
assert.deepStrictEqual(instance.name, 'foo');
}

{
// Test get instance
const instance = await env.workflow.get('bar');
assert.deepStrictEqual(instance.id, 'bar');
assert.deepStrictEqual(instance.name, 'bar');
}
},
};
6 changes: 4 additions & 2 deletions src/cloudflare/internal/test/workflows/workflows-mock.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ export default {
return Response.json(
{
result: {
instanceId: data.id,
instanceId: 'id',
instanceName: data.name,
},
},
{
Expand All @@ -26,7 +27,8 @@ export default {
return Response.json(
{
result: {
instanceId: data.id,
instanceId: 'id',
instanceName: data.name,
},
},
{
Expand Down
38 changes: 23 additions & 15 deletions src/cloudflare/internal/workflows-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,11 @@ async function callFetcher<T>(
class InstanceImpl implements Instance {
private readonly fetcher: Fetcher;
public readonly id: string;
public readonly name: string;

public constructor(id: string, fetcher: Fetcher) {
public constructor(id: string, name: string, fetcher: Fetcher) {
this.id = id;
this.name = name;
this.fetcher = fetcher;
}

Expand Down Expand Up @@ -93,24 +95,30 @@ class WorkflowImpl {
this.fetcher = fetcher;
}

public async get(id: string): Promise<Instance> {
const result = await callFetcher<{ instanceId: string }>(
this.fetcher,
'/get',
{ id }
);
public async get(name: string): Promise<Instance> {
const result = await callFetcher<{
instanceId: string;
instanceName: string;
}>(this.fetcher, '/get', { name });

return new InstanceImpl(result.instanceId, this.fetcher);
return new InstanceImpl(
result.instanceId,
result.instanceName,
this.fetcher
);
}

public async create(id: string, params: object): Promise<Instance> {
const result = await callFetcher<{ instanceId: string }>(
this.fetcher,
'/create',
{ id, params }
);
public async create(name: string, params: object): Promise<Instance> {
const result = await callFetcher<{
instanceId: string;
instanceName: string;
}>(this.fetcher, '/create', { name, params });

return new InstanceImpl(result.instanceId, this.fetcher);
return new InstanceImpl(
result.instanceId,
result.instanceName,
this.fetcher
);
}
}

Expand Down
11 changes: 6 additions & 5 deletions types/defines/workflows.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ declare module "cloudflare:workflows" {
declare abstract class Workflow {
/**
* Get a handle to an existing instance of the Workflow.
* @param id Id for the instance of this Workflow
* @param name Name of the instance of this Workflow
* @returns A promise that resolves with a handle for the Instance
*/
public get(id: string): Promise<Instance>;
public get(name: string): Promise<Instance>;

/**
* Create a new instance and return a handle to it. If a provided id exists, an error will be thrown.
* @param id Id to create the instance of this Workflow with
* Create a new instance and return a handle to it. If a provided instance name exists, an error will be thrown.
* @param name Name to create the instance of this Workflow with
* @param params The payload to send over to this instance
* @returns A promise that resolves with a handle for the Instance
*/
public create(id: string, params: object): Promise<Instance>;
public create(name: string, params: object): Promise<Instance>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to create a CreateInstanceParams interface, to allow for better extensibility (we are making breaking changes anyway)

Copy link
Contributor

@LuisDuarte1 LuisDuarte1 Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params should be unknown and not an object

}

type InstanceStatus = {
Expand All @@ -49,6 +49,7 @@ interface WorkflowError {

declare abstract class Instance {
public id: string;
public name: string;

/**
* Pause the instance.
Expand Down