-
Notifications
You must be signed in to change notification settings - Fork 25
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
docs(examples): hello world, prompt #84
Conversation
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.
early feedback, I haven't looked at the handle prompt example yet.
I would suggest we remove 1-
prefixes, the names should stay on their own
examples/1-hello-world/package.json
Outdated
}, | ||
"scripts": { | ||
"start": "node index.js", | ||
"watch": "node --watch index.js" |
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.
does node support a --watch
flag now?
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.
Yep
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.
woah today I learned, thanks!
https://nodejs.org/api/cli.html#--watch
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.
Copilot told me it was available!
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.
not sure if we should commit lock files for the examples, they might just create a lot of noise
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.
Fair point, I'll remove them
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.
Removed
examples/1-hello-world/index.js
Outdated
} from "@copilot-extensions/preview-sdk"; | ||
|
||
const server = createServer((request, ressponse) => { | ||
console.log(`Received [${request.method}]`); |
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 think we should log out both the method and the URL
console.log(`Received [${request.method}]`); | |
console.log(`Received ${request.method} ${request.url}`); |
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 you removed the brackets as well. It's a preference of mine to be able to differentiate between fixed text and variable text, and makes it really easy to see when a value is empty.
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.
we can leave in the brackets if you prefer. But values cannot be empty in this context
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.
Added
I was aiming for having a very logical way of knowing where to start and gradually add more complex examples. Numbering them helps so that you do not need to search in the README. |
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
package.json
Outdated
@@ -21,6 +21,7 @@ | |||
"license": "MIT", | |||
"description": "JavaScript SDK for Copilot Extensions", | |||
"dependencies": { | |||
"@copilot-extensions/preview-sdk": "^4.0.3", |
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.
did you add this dependency on itself intentionally? If yes, why?
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.
No, probably ran an npm install command at the wrong folder. Removed it!
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.
Resolved feedback comments in the code
examples/1-hello-world/index.js
Outdated
} from "@copilot-extensions/preview-sdk"; | ||
|
||
const server = createServer((request, ressponse) => { | ||
console.log(`Received [${request.method}]`); |
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.
Added
package.json
Outdated
@@ -21,6 +21,7 @@ | |||
"license": "MIT", | |||
"description": "JavaScript SDK for Copilot Extensions", | |||
"dependencies": { | |||
"@copilot-extensions/preview-sdk": "^4.0.3", |
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.
No, probably ran an npm install command at the wrong folder. Removed it!
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.
Removed
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.
sorry for all the comments. If you prefer, we can merge it as is and do a follow up pull request to discuss further, or leave out the 2nd example folder and do it in a separate pull request
examples/2-handle-prompt/index.js
Outdated
@@ -0,0 +1,100 @@ | |||
import http from 'http'; | |||
import { Octokit } from "@octokit/rest"; |
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.
Do not use @octokit/rest
, use octokit
instead. I know Copilot likes to recommend @octokit/rest
because it's been around longer, but it's a legacy SDK. octokit
is the official all-batteries-included SDK for GitHub's API now
examples/2-handle-prompt/index.js
Outdated
|
||
// Define the handler function | ||
const handler = async (request, response) => { | ||
if (request.method === 'POST') { |
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 would reverse condition and do an early return to avoid indentation
if (request.method === 'POST') { | |
if (request.method !== 'POST') { |
examples/2-handle-prompt/index.js
Outdated
// header to indicate the state | ||
response.writeHead(200, { 'Content-Type': 'application/json' }); |
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 don't think this is needed. The content is not valid JSON code anyway
examples/2-handle-prompt/index.js
Outdated
// parse the incoming body as that has the information we need to handle the request / user prompt | ||
const payload = parseRequestBody(body); |
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 think I would create a separate method to read the body text instead of moving all the core logic into the request.on('end', async () => {
handler. And in this example, we should verify the payload using https://github.com/copilot-extensions/preview-sdk.js/?tab=readme-ov-file#async-verifyrequestbykeyidrawbody-signature-keyid-requestoptions
examples/2-handle-prompt/index.js
Outdated
// add new lines to mark the difference between the fixed text and the dynamic text | ||
response.write("\n\n"); |
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.
the new lines are included in textEvent
// add new lines to mark the difference between the fixed text and the dynamic text | |
response.write("\n\n"); |
examples/2-handle-prompt/index.js
Outdated
// add a first system prompt for the payload.messages to add instructions | ||
payload.messages.unshift({ role: "system", content: "You are a helpful assistant that talks like a pirate." }); |
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.
WE are doing this system message unshifting inside prompt
already
// add a first system prompt for the payload.messages to add instructions | |
payload.messages.unshift({ role: "system", content: "You are a helpful assistant that talks like a pirate." }); |
examples/2-handle-prompt/index.js
Outdated
const payload_message = payload.messages[payload.messages.length - 1]; | ||
const result = await prompt(payload_message.content, { | ||
messages: payload.messages, // we are giving the prompt the existing messages in this chat conversation | ||
model: "gpt-4", |
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.
setting model
is optional, we default it to gp-4o
right now
Line 4 in b7063d6
const DEFAULT_MODEL = "gpt-4o"; |
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
@gr2m , I've addressed most of the feedback but moving the body parsing logic out of the |
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.
Let me know if you had a chance to test the apps once you applied all the remaining requested changes. If not I can do that too
examples/2-handle-prompt/index.js
Outdated
// the prompt to forward to the Copilot API is the last message in the payload | ||
const payload_message = payload.messages[payload.messages.length - 1]; | ||
const result = await prompt(payload_message.content, { | ||
system: "you talk like a pirate", // extra instructions for the prompt, the "you are a helpful assistant" is already set in the SDK |
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.
The "you are a helpful assistant" is just the default. If you want it to be part of your system prompt then you have to include it in your custom onew
system: "you talk like a pirate", // extra instructions for the prompt, the "you are a helpful assistant" is already set in the SDK | |
system: "You are a helpful assistant. You talk like a pirate", |
examples/2-handle-prompt/index.js
Outdated
// write the prompt response back to Copilot | ||
// note that this is only send when the entire response from the Copilot API is ready | ||
response.write(createTextEvent(result.message.content)); |
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.
there is a prompt.stream
API but we can do a separate example for that if you like?
}, | ||
"dependencies": { | ||
"@copilot-extensions/preview-sdk": "latest", | ||
"@octokit": "latest" |
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.
Don't use latest
for external dependencies.
"@octokit": "latest" | |
"octokit": "^4.0.2" |
We have a live stream today that I think might draw some interest, I think it would be great to finish this up by then: I'll address my remarks to get the PR merged, we can do follow up PRs aftewards |
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.
Thank you for getting an examples/
folder started, this will be tremendously helpful. I should have started out with that 🙏🏼
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
See the conversation in #83
examples
folder with easy to start projects (seeREADME
in that folder)