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

Add methods for shaping response events #3

Closed
alexisabril opened this issue Aug 27, 2024 · 5 comments · Fixed by #32
Closed

Add methods for shaping response events #3

alexisabril opened this issue Aug 27, 2024 · 5 comments · Fixed by #32
Assignees
Labels

Comments

@alexisabril
Copy link

alexisabril commented Aug 27, 2024

Helper method(s) could be defined to assist in the shaping of a Copilot Chat response or the streaming of the response. Response shapes are a common issue, with inconsistencies from developers around:

  • specifying types
  • how to break apart chunks
  • proper formatting of chunks
  • proper termination of response
  • what types of chunks can be grouped in a singular response
@gr2m
Copy link
Collaborator

gr2m commented Aug 27, 2024

I started some dreamcoding. It doesn't answer the questions of how yet, but it helps me understand if this is the direction we want to go. I'd love to hear your thoughts:
https://github.com/copilot-extensions/preview-sdk.js/blob/main/dreamcode.md

In terms of shaping responses, I'm thinking to have set of methods matching our types, like text (for default), confirmation, etc (haven't looked further yet 😅 ).

The low-level methods could work two fold. For one, we have methods for each type that creates respond objects, e.g.

const confirmationObject = confirmation({
  title: "Would you like to hear a joke?",
  message: "I have a joke about construction, but I'm still working on it.",
  id: "joke",
  // optional
  meta: {
    other: "data",
  },
});

Where confirmationObject would look like this

{
  "event": "copilot_confirmation",
  "data": {
      "type": "action",
      "title": "Would you like to hear a joke?",
      "message": "I have a joke about construction, but I'm still working on it.",
      "confirmation": {
          "id": "joke",
          "other": "data"
      }
  }
}

and then a separate method which takes these objects that we support and turn them into an SSE-compatible line

const line = toServerSideEventLine(confirmationObject)

where line would then look like this (note the two \n\n at the end 😅 )

event: copilot_confirmation
data: {
	"type": "action",
	"title": "Would you like to hear a joke?",
	"message": "I have a joke about construction, but I'm still working on it.",
	"confirmation": {
		"id": "joke",
		"other": "data",
	}
}


The line could then be directly passed into a standard node response object

response.write(line)

This is really just me thinking out loud, usually I'd think more before bothering folks with notifications. But given the short timeline there is no time for such niceties 🙇

@gr2m
Copy link
Collaborator

gr2m commented Aug 28, 2024

Had a chat with @alexisabril and he suggested to add a .toString() method to the objects returned by response building block methods such as confirmation. So instead of

const confirmationObject = confirmation({
  title: "Would you like to hear a joke?",
  message: "I have a joke about construction, but I'm still working on it.",
  id: "joke",
});
const line = toServerSideEventLine(confirmationObject)
response.write(toServerSideEventLine)

we could do

const confirmationObject = confirmation({
  title: "Would you like to hear a joke?",
  message: "I have a joke about construction, but I'm still working on it.",
  id: "joke",
});
response.write(confirmationObject) // calls `confirmationObject.toString()`

I like it!!

@gr2m gr2m changed the title Methods for shaping responses [BATCH] Add methods for shaping responses Aug 29, 2024
@gr2m gr2m self-assigned this Aug 29, 2024
@gr2m gr2m changed the title [BATCH] Add methods for shaping responses Add methods for shaping response events Aug 29, 2024
@gr2m
Copy link
Collaborator

gr2m commented Aug 29, 2024

I added the list of methods I want to build as sub issues. The list as of now is

  • createAckEvent()
  • createTextEvent()
  • createDoneEvent()
  • createConfirmationEvent()
  • createReferencesEvent()
  • createErrorEvent()

I thought createAck() or just ack() is to ambivalent, especially for createError(). Appending Event makes it more clear. Appending ResponseEvent() might make it even more clear, and I don't mind long method names as long as they serve clarity.

Any thoughts?

@alexisabril
Copy link
Author

This is something I struggle with myself and I could lean either way. Do we have any precedence in Octokit today for these types of methods? Thoughts that occur to me:

  • If this is new and standalone, I'm in favor of the above
  • If this is nested, such as ...copilot.events.createAckEvent(), then we could potentially remove Event from the name if the parent shares the name
  • I'm a fan of the scheme createAckEvent in general vs a Parent.createX signals to me some type of other interaction, such as proto-chain wiring or maintaining state.

The above are my raw thoughts, but tldr; I'm leaning towards the proposed API unless this would introduce inconsistency with an existing Octokit API.

Copy link

🎉 This issue has been resolved in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants