-
Notifications
You must be signed in to change notification settings - Fork 45
fix: typescript definitions #72
fix: typescript definitions #72
Conversation
Please look :) |
9b72652
to
c70e2b7
Compare
c70e2b7
to
475821c
Compare
src/client.d.ts
Outdated
export interface Message { | ||
role: string; | ||
// TODO: this is not specified at https://docs.mistral.ai/redocusaurus/plugin-redoc-0.yaml | ||
name?: string, |
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.
Normally there is no name
on the message, I think we can remove it.
src/client.d.ts
Outdated
function: FunctionCall; | ||
} | ||
|
||
export enum ResponseFormats { |
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 also remove the enums from the types and change them to types. A simple example like this does not work because the enums are TS only:
import MistralClient, { ResponseFormats } from "@mistralai/mistralai";
const chatResponse = await mistralClient.chat({
model: "mistral-large",
temperature: 0.1,
responseFormat: { type: ResponseFormats.json_object },
})`
This raises the error 'mistralai/src/client.js' does not provide an export named 'ResponseFormats'
I suppose someone put "name" in there for OpenAI compat reasons at some
point, though it is just misleading, and also not necessary
…
> Message ID: ***@***.***>
>
|
Oh right, yeah, the enums aren't even defined at runtime are they. I added
literals (for ergonomics) and left the enums in there and moved on. They
are completely broken though aren't they.
A good argument to move to TS :)
…On Tue, 7 May 2024, 2:51 pm Nicholas Dudfield, ***@***.***> wrote:
>
> I suppose someone put "name" in there for OpenAI compat reasons at some
point, though it is just misleading, and also not necessary
>
>> Message ID: ***@***.***>
>>
>
—
Reply to this email directly, view it on GitHub
<#72 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEAHG6QYRPI5CS72RB4DX3ZBCBWLAVCNFSM6AAAAABHKCIQK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJXGY3DSOJUHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
In a |
Anything else? I'll look at a full TS port when I have more time |
Oh, wait, that's not working, enums still referenced, but no errors, hrmmm |
export interface ToolCalls { | ||
id: 'null'; | ||
type: ToolType = ToolType.function; | ||
function: FunctionCall; |
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 happened here? :)
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 do you mean?
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.
id: 'null';
type: ToolType = ToolType.function;
As you mentioned, the enums don't work at runtime, and null
is set to match the string null, not the null type (which is actually not appropriate either). Also, it's trying to set a default (ToolType = ToolType.function;
) inside a .d.ts
file, which I don't believe actually works.
More a rhetorical question than serious :) Wondering how 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.
Yeah the null
string is expected, as before our API change the id returned by the API was always 'null'
(the string). Now it is function call ids yes. The default stuff well I'm not sure ahah
In a separate PR, to be clear edit: the question is if you all want to merge this ( I think it makes sense, as it's still progress, and closes some issues), or wait for a full TS port |
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.
Thanks a lot @sublimator!
This should make the typescript definitions at least useable. Currently they seem to be quite broken (per #69, #67 )
Closes #54
Closes #47
Closes #69
Closes #67
Closes #62