-
Notifications
You must be signed in to change notification settings - Fork 51
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
End-to-end integration fixes #5
Conversation
76356d3
to
ff35604
Compare
agents/src/worker.ts
Outdated
@@ -119,6 +120,15 @@ class ActiveJob { | |||
} | |||
} | |||
|
|||
class PendingAssignment { | |||
promise = new Promise<JobAssignment>((resolve) => { | |||
this.resolve = resolve; // oh, JavaScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is JavaScript's way of triggering a Promise.resolve outside of a Promise. it looks wrong but that's the way you're supposed to do it
later in the code we do PendingAssignment.promise.await
the reason PendingAssignment.resolve(arg: JobAssignment)
isn't empty is because of ESLint type checking, otherwise it complains about unused-variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you are trying to do.. perhaps we should make this helper a shared utils lib somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm... we could make a @livekit/common
helper library, but i don't see real use for it unless there's a lot of shared code between the projects
the cost of importing a new library outweighs the cost of redundancy up to a certain point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either way this can be addressed later, i'm going to merge
(this was merged, i just did it wrong) |
i'm finally out of integration hell 🎉 general changes to the whole control flow, making things that errored not error anymore, work from start to finish, etc.
manually tested and confirmed to be pretty much functionally equivalent to livekit/agents, from the server side at least. the logs also behave in the same way. tested using agents-playground both using LK Cloud and local livekit-server.
notable missing implementation detail is multi-processing and job isolation, which will be done shortly
closes #2