fix: upgrade @copilot-extensions/preview-sdk to v2
#5
Conversation
src/index.ts
Outdated
| return; | ||
| } | ||
|
|
||
| response.write(createAckEvent().toString()); |
There was a problem hiding this comment.
I don't love the idea of this method - it feels unnecessary, and "Ack" isn't a super common term. The functionality of the integration doesn't change at all without calling this.
Separately: can we make the create*Event methods just return a string? Is there a case where you need the object returned, and not just the string?
There was a problem hiding this comment.
The functionality of the integration doesn't change at all without calling this.
it does put the chat UI into "the agent received your request and is responding" mode, the UI changes. I would consider it best practice. But we can remove it if you prefer
Separately: can we make the
create*Eventmethods just return a string? Is there a case where you need the object returned, and not just the string?
These are super low-level methods. I was hoping that response.write would call the .toString() method directly, but alas, it doesn't. I think it makes sense for them to create an object in case you want to use your own logic to transport them. Once they are a string, the structural data can no longer be retrieved.
The higher-level API would make the necessity of calling .toString() go away anyway though:
https://github.com/copilot-extensions/preview-sdk.js/blob/9533a1fd20409332c8f1cf60ac15e2421347ce55/dreamcode.md#L35-L43
There was a problem hiding this comment.
you know what I changed my mind, let's just have them generate strings, it's a problem when it's a problem. I'll apply the change in #6 if that's okay with you, let's get that one merged first, ok?
There was a problem hiding this comment.
and "Ack" isn't a super common term
yeah naming is hard 🤔 What would you call it? Maybe avoid the abbreviation createAcknowledgeEvent? Or call it createConfirmRequestEvent()? The latter is too close to createConfirmationEvent() for my taste though
There was a problem hiding this comment.
Not opposed to this returning strings. I don't have a good stance on if this should be an object, unless there's a primary adoption for something consuming this that would need a certain shape. Folks will be passing in objects to these methods, in most cases, so they should have a handle to the non-string version if needed, before these methods are called.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'll go with createAcknowledgementEvent() for now, more coherent than createAcknowledgeEvent() with the other create*Event() methods
There was a problem hiding this comment.
it does put the chat UI into "the agent received your request and is responding" mode, the UI changes. I would consider it best practice. But we can remove it if you prefer
This points to an improvement we should make to the platform; we already know when we've started communicating to an external agent. I'd prefer to remove that for now if that's cool!
There was a problem hiding this comment.
I removed it via 7d5dd21
src/index.ts
Outdated
| // @ts-expect-error - TODO @gr2m - type incompatibility between @openai/api and @copilot-extensions/preview-sdk | ||
| messages: toolCallMessages, |
There was a problem hiding this comment.
Why? Why wouldn't we make the preview-sdk's types compatible?
Moreover: I'm not sure I agree that the SDK should contain an SDK for OpenAI. Like in #6, we're also calling the GitHub Models API - that's an OpenAI-compatible API, and has nothing to do with Copilot. I wouldn't expect or want to use a Copilot SDK for that.
There was a problem hiding this comment.
the prompt() method originated from discussions in GitHub Models, where we wanted to simplify the getting started documentation by using our own SDK that removes some of the complexities. However that related to Azure's SDKs.
I can bring back the OpenAI SDK if you prefer, it's just quite a heavy dependency with a lot of functionality you don't need here
There was a problem hiding this comment.
it's just quite a heavy dependency with a lot of functionality you don't need here
I don't really agree - and it's more a question of what makes most sense for the models extension vs. all Copilot Extensions. As an Extension that calls multiple models, the more robust and well-used SDK feels more appropriate. Besides which, for an example repository I'd bias more towards clarity and adherence to the ecosystem over performance or dependency weight.
Co-authored-by: Jason Etcovitch <[email protected]>
I tested it locally 👍🏼