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

Initial doc for TSV2 #268

Merged
merged 12 commits into from
Dec 11, 2024
Merged

Initial doc for TSV2 #268

merged 12 commits into from
Dec 11, 2024

Conversation

chuck-dbos
Copy link
Contributor

Initial reference docs for TS V2 API

Fix a couple of small issues w/ older docs

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to put a warning at the top saying this page is for v1? Then redirect people to the newer version.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move the old reference docs like DBOS Decorators, DBOS Contexts, and Testing Runtime into a separate folder?

Copy link
Member

@kraftp kraftp Dec 6, 2024

Choose a reason for hiding this comment

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

Agreed. This is a fresh start in my opinion, although we maintain compatibility with the old version. I would totally remove all references to the old version (or bury them in a separate folder) then split up this huge file. That way we get a much cleaner structure as opposed to 6 old pages and one gigantic new page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this as a phased process... Having the API and this material as a reference, publishing to npm. Then having the examples and changing all the tutorial sections to do the new way as a follow-on. In case we find any issues, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to get the TS docs well-structured before we officially "launch" this, right now they're hard to follow. In my opinion that would mean a smaller number of tutorials that are totally focused on the new version. No one is going to use the new version based only on the API reference, we have to transition the tutorials and templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agree, I think it's a second phase after this one.

DBOS.withNextWorkflowID<R>(wfid: string, callback: ()=>Promise<R>) : Promise<R>
```

The `DBOS.withNextWorkflowID` function runs the provided `callback` function and returns its result. The first workflow started within `callback` will be assigned the ID specified by `wfid`.
Copy link
Member

@qianl15 qianl15 Dec 10, 2024

Choose a reason for hiding this comment

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

The callback function currently allows running multiple workflows with the same workflow ID(?) Because you could put any arbitrary code within the async() => {} block. Can we have a clear error message to disallow invoking multiple workflows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we'd clear the workflow ID off after use.


```typescript
DBOS.sleepms(durationMS: number): Promise<void>
DBOS.sleep(durationSec: number): Promise<void>
Copy link
Member

Choose a reason for hiding this comment

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

Is now a good time to change DBOS.sleep to use millisecond by default? It's a new interface anyways...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Approved!

```
`DBOS.send()` to send a message to a workflow, identified by `destinationID`. Messages can optionally be associated with a topic and are queued on the receiver per topic. `DBOS.send` may be called from other workflows, or anywhere else, to pass information to a corresponding `DBOS.recv` call.

For more information, see our [messages API tutorial](../tutorials/workflow-communication-tutorial#messages-api).
Copy link
Member

Choose a reason for hiding this comment

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

This link is about using contexts from v1. Maybe remove this reference for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we would rewrite this, and other things that we're referring to, in the next round - up for review early next week and merged soon thereafter. A link to the old one is better than none, for now.

### Decorators
Such libraries may choose to implement method decorators. If the method decorator is registering a function with DBOS Transact, it should call `DBOS.registerAndWrapContextFreeFunction` for functions that do not accept a context as the first parameter.

```typescript
Copy link
Member

Choose a reason for hiding this comment

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

Would benefit from having an example here to show how to define a custom decorator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be tried before we'd have that. I think we deprioritized custom event receivers.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, let's deprioritize it. Maybe even cut this "Extensibility -> Decorators" section for now.

)
```

`DBOS.registerAndWrapContextFreeFunction` registers the function named `propertyKey` on the `target` class, creates a wrapper function, and updates the `descriptor` to use the wrapper function.
Copy link
Member

Choose a reason for hiding this comment

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

The user-facing name could be shorter, like DBOS.createDecorator or so...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above - I think when we try this we will change it a bit. createDecorator is not right, though.

Copy link
Member

@kraftp kraftp Dec 11, 2024

Choose a reason for hiding this comment

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

Let's cut this section for now. These features aren't a priority.


```typescript
// Set the authenticated user, and roles allowed by the authentication system for the scope of the callback() function
DBOS.withAuthedContext<R>(authedUser: string, authedRoles: string[], callback: () => Promise<R>): Promise<R>
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of having separate contexts, can we create a single context type with the auth, trace, and caller name fields? Then, it will also allow users to attach their custom fields into the context object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could possibly be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also see comment above about needing more real experience with it before the API and doc will be very good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the idea here was that these augment any existing context in certain ways, or start a new one. We could allow use of runWithTopContext, this does what you want but isn't exported from DBOS right now.

Copy link
Member

@kraftp kraftp Dec 11, 2024

Choose a reason for hiding this comment

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

Not a priority, honestly let's just remove the entire "Extensibility" section and come back to it later once we have more feedback/experience. Main focus should be on the core interface.

DBOS.withWorkflowQueue<R>(wfq: string, callback: ()=>Promise<R>) : Promise<R>
```

The `DBOS.withWorkflowQueue` function runs the provided `callback` function and returns its result. Any workflows started within `callback` will be run on the queue named by `wfq`.
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, this is being replaced with the new interface from dbos-inc/dbos-transact-ts#673, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Augmented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, the whole neat thing about v2 is that it just looks like function calls. That can now use queues to throttle the app pace, which is pretty cool if you think about it.

Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't understand the callback-based queues interface. I can see how to use it, but I don't see why it needs a callback. The new interface based on DBOS.startWorkflow makes a lot more sense. Are there advantages to the callback-based interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placed next to each other for a startWorkflow, no, it doesn't make much sense.
Without the startWorkflow, it's a wash, because you have the callback or you have the await handle.getResult
Placed apart, say as a middleware, or administrative policy, then could you see it? Because that's where I see the value. Run the workflows in this class on this queue, or on these HTTP routes in that queue, etc.

Copy link
Member

@kraftp kraftp Dec 11, 2024

Choose a reason for hiding this comment

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

I can see how you could use it, but I don't think it's enough to justify having two ways of doing it. Would rather keep things simple and support/document only the new interface.

Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

This looks good. I love the new interface. The big issue is the queues interface, but it looks like that's addressed in dbos-inc/dbos-transact-ts#673.

I also think we should totally cut the "Extensibility" section. Those features are not high-priority and are still half-baked, so better to cut them for now and add them in later when we're more confident in them.

@chuck-dbos
Copy link
Contributor Author

I like the plan to cut the extensibility part. I will just commit that onto a separate branch, so it isn't lost.

@chuck-dbos chuck-dbos merged commit 36a44ca into main Dec 11, 2024
1 check passed
@chuck-dbos chuck-dbos deleted the chuck/tsv2 branch December 11, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants