-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(attachments): create context-aware helper for attachments #44
feat(attachments): create context-aware helper for attachments #44
Conversation
ENG-1590 TS SDK : inconsistencies on uploadFile
The
For the content part, it is a bit impractical when the file comes from a web form. In this case it is presented to you as a
In a similar situation, the As a general rule we want to change the API for attachments, instead of first uploading the file then creating the Attachment instance we should have
|
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.
Looks great to me!
tests/chainlit-logo.png
Outdated
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.
It's still a png, what was the trick to reduce size? # of RGB channels?
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.
yes ! i used one of those online squashers, and reduced the color depth (to <10 colors iirc)
@@ -596,19 +620,25 @@ export class API { | |||
* @returns An object containing the `objectKey` of the uploaded file and the signed `url`, or `null` values if the upload fails. | |||
* @throws {Error} Throws an error if neither `content` nor `path` is provided, or if the server response is invalid. | |||
*/ | |||
|
|||
async uploadFile(params: UploadFileParamsWithContent): Promise<{ |
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 didn't know we could declare prototypes for doc/autocompletion purposes.
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.
Just realized you mentioned "Fix typing to enforce either content or path at compile time" -> very clear.
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.
yes overloading function types is a great way to handle this kind of either/or parameters. It's a bit verbose but the end result is satisfying from the POV of the dev using the function.
@@ -0,0 +1,96 @@ | |||
import 'dotenv/config'; |
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 so much for splitting tests in their own file!
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.
you're welcome, i'm unable to focus on a 700 line test file anyway 😅
tests/attachments.test.ts
Outdated
const arrayBuffer = buffer.buffer; | ||
const blob = new Blob([buffer]); | ||
// We wrap the blob in a blob and simulate the structure of a File | ||
const file = new Blob([blob], { type: 'image/jpeg' }); |
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.
png maybe?
src/api.ts
Outdated
try { | ||
threadFromStore = this.client.getCurrentThread(); | ||
} catch (error) { | ||
// Ignore error thrown if getCurrentThread is called outside of a 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.
When I debug, I expect caught errors to appear somewhere in the logs, it helps me navigate through the code and quickly identify the location of a bug. I think it's only me in the team though.
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.
In this case, there is genuinely nothing to log. You may or may not be inside of a context, both of which are valid states. But you're right, that's a code smell so i added _currentThread
and _currentStep
methods which do not throw outside of context. It is much cleaner.
I couldn't make them private (as they are consumed from outside of the LiteralClient class) but they are undocumented.
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, it's like using try/catch as an if condition.
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 the openai e2e test should be impacted by this change, there are no attachments in those tests.
yes, confirmed, they do work in local |
Non-Breaking
This creates a new context-aware helper that abstracts the logic around creating attachments, as well as providing the following enhancements :
Old syntax
New syntax