Feature/ts tsup #73

JamieLee0510 · 2024-05-08T04:42:17Z

this PR is for refactor original javascript version to typescript + tsup;
also contain test cases, but example cases not yet.

JamieLee0510 · 2024-05-13T07:59:38Z

Have updated this pr from current diff

fuegoio

Thank you very much for your work! 🎉

Some high-level note: we should I think remove jest and replace it by vitest, especially to get rid of babel as a dependency.

src/utils/init-fetch.ts

fuegoio · 2024-05-13T08:13:08Z

src/utils/type.ts

Could you combine the JSDoc with the Typescript definitions please :)? You need to add the fields description as comments above fields in the TS def.

fuegoio · 2024-05-13T08:14:58Z

tests/client.test.ts

      const mockResponse = mockChatResponsePayload();
-      client._fetch = mockFetch(200, mockResponse);
+      globalThis.fetch = mockFetch(200, mockResponse) as any;


See my comment above but let's use a private fetch only for our package!

fuegoio · 2024-05-13T08:16:12Z

tests/test-utils.ts

+    total_tokens: number;
+    completion_tokens: number;
+  };
+}


Why do you need to put the interfaces again here?

just clean it in further commit.

fuegoio · 2024-05-13T08:17:01Z

tsup.config.ts

+    shims:true,
+    skipNodeModulesBundle:true,
+    clean:true
+})


Does it build with source maps as well?

yes could build source map in tsup.

JamieLee0510 · 2024-05-14T04:09:33Z

Some high-level note: we should I think remove jest and replace it by vitest, especially to get rid of babel as a dependency.

Currently I still use jest but get rid of babel by jest config setting preset: 'ts-jest/presets/default-esm', which could work fine as well.

please kindly help check the updated PR :)

sublimator · 2024-05-14T10:32:54Z

src/mistral-client.ts

-    this.endpoint = endpoint;
-    this.apiKey = apiKey;
+export default class MistralClient {
+  private apiKey: string;


for better or worse, these were public fields in the .d.ts file from 0.2

sublimator · 2024-05-14T10:34:03Z

src/mistral-client.ts

   */
-  async _fetch(...args) {
+  public async _fetch(input: string | Request, init?: RequestInit){


This should be private. You can access it in the tests as client['_fetch']

sublimator · 2024-05-14T10:35:18Z

src/mistral-client.ts

              return response.body;
-            } else {
-              const reader = response.body.getReader();
+            }else {


Looks like formatting needs running? }els {

this one I think it might need something like .prettierrc config to align formatting afterwards.
I have noticed that there was a pr related with single-quote.

sublimator · 2024-05-14T10:36:57Z

src/mistral-client.ts

-          await new Promise((resolve) =>
-            setTimeout(resolve, Math.pow(2, (attempts + 1)) * 500),
-          );
+          console.debug(`Retrying request on response status: ${response.status}`);


You've dropped some of the text here:
"""
Response: ${await response.text()},
Attempt: ${attempts + 1},
"""

JamieLee0510 · 2024-05-14T11:56:09Z

all issues raised by @sublimator in previous batch have been resolved, thanks :)

sublimator · 2024-05-14T12:00:19Z

On mobile now, but I think the throw branch also included resp.text()

…

On Tue, 14 May 2024, 6:56 pm Jamie Lee, ***@***.***> wrote: all issues raised by @sublimator <https://github.com/sublimator> in previous batch have been resolved, thanks :) — Reply to this email directly, view it on GitHub <#73 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEAHG5C7E22J5BUAJ2CAIDZCH3W7AVCNFSM6AAAAABHMGW7YCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGAYTINBRHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

sublimator · 2024-05-14T12:00:59Z

And yeah, prettier would rock! Not sure if lint --fix still works now TS

…

On Tue, 14 May 2024, 7:00 pm Nicholas Dudfield, ***@***.***> wrote: On mobile now, but I think the throw branch also included resp.text() On Tue, 14 May 2024, 6:56 pm Jamie Lee, ***@***.***> wrote: > all issues raised by @sublimator <https://github.com/sublimator> in > previous batch have been resolved, thanks :) > > — > Reply to this email directly, view it on GitHub > <#73 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEAHG5C7E22J5BUAJ2CAIDZCH3W7AVCNFSM6AAAAABHMGW7YCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGAYTINBRHA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

sublimator · 2024-05-14T12:15:02Z

src/utils/helper.ts

+ });
+
+ return controller.signal;
+}


Seem it wants you to add an extra line at the end

sublimator · 2024-05-14T12:16:39Z

tests/test-utils.ts

@@ -1,12 +1,20 @@
-import jest from 'jest-mock';
+import { jest } from '@jest/globals';
+import { ChatCompletionResponse, Embedding, EmbeddingResponse, ListModelsResponse } from "@mistralai/mistralai/utils/type"


Somehow surprised that import works

sublimator · 2024-05-14T12:22:15Z

tests/test-utils.ts

- * @param {number} batchSize
- * @return {Object}
+ * @param batchSize number of embeddings to generate
+ * @returns EmbeddingResponsePayload


The linter isn't crying about returns (vs return)? I guess not. I thought I recalled it doing so for me.

sublimator · 2024-05-14T12:22:32Z

tests/test-utils.ts

- * @return {Object}
- */
-export function mockEmbeddingRequest() {
+// TODO:


Undone TODO

sublimator · 2024-05-14T12:23:02Z

tsconfig.json

+      "outDir": "dist",
+      "baseUrl": "./",
+      "paths": {
+        "@mistralai/mistralai/*": ["src/*"],


That's how that import worked!

sublimator · 2024-05-14T12:25:13Z

src/mistral-client.ts

@@ -1,132 +1,121 @@
-const VERSION = '0.0.3';


Ideally this PR would be based on top of the latest commit in main. It doesn't seem to be, given VERSION of 0.0.3 ?

but it is set 0.2.0 in latest commit in main branch:

client-js/src/client.js

Line 1 in 2fcc564

const VERSION = '0.2.0';

or anything I misunderstand?

Right, when looking at the diff for this PR we are seeing 0.0.3
That's because this branch is commits based on an old version of main
It complicates review a little bit

sublimator · 2024-05-14T12:28:55Z

src/utils/helper.ts

+export function combineSignals(signals: AbortSignal[]):AbortSignal {
+ const controller = new AbortController();
+ signals.forEach((signal) => {
+   if (!signal) {


oh, yeah, it was taking an array of AbortSignal | undefined

sublimator · 2024-05-15T10:41:51Z

src/mistral-client.ts

-            `HTTP error! status: ${response.status} ` +
-            `Response: \n${await response.text()}`,
-          );
+          throw new MistralAPIError(`HTTP error! status: ${response.status}`);


You're missing Response: \n${await response.text()} from the og

sublimator · 2024-05-15T10:43:24Z

src/mistral-client.ts


  /**
   * Returns a list of the available models
-   * @return {Promise<Object>}
+   * @returns 


consistent return vs returns?

Personally I would just lose the JSDoc now that it's typescript

due to this discussion:
#73 (comment)

I agree with jsdoc making code more readable, but okay with both.

JamieLee0510 · 2024-05-16T07:25:15Z

Is it possible to deal with code formatting in other PR?
I think it could add other config file after discussion.

jwerre · 2024-06-07T20:45:23Z

Looking forward to this! Lack of Commonjs support has prevented me from using this.

JamieLee0510 · 2024-06-08T06:58:48Z

Looking forward to this! Lack of Commonjs support has prevented me from using this.

sure let me catch up the latest commit and merge into this pr
hope Mistral team could approve this pr

JamieLee0510 · 2024-06-08T10:06:44Z

@fuegoio
Please kindly help check the updated version.
This version is also fetched up job and file features~

GaspardBT · 2024-10-10T11:26:53Z

Thanks for opening this PR. We have deprecated this package in favor of mistralai/client-ts, which is the new official Mistral client, compatible with both TypeScript and JavaScript.

You can find all installation information here.

This change is effective starting with version 1.0.0 of the npm package.

JamieLee0510 added 5 commits May 8, 2024 11:01

feat: typescript setting

14b804e

feat: core javascript script refactor in typescript

2ab0f15

feat: build setting

2e8a4a4

feat: test cases and related setting

85f667a

fix: cjs and test issue

6666d59

JamieLee0510 mentioned this pull request May 8, 2024

TypeScript or Babel #70

Closed

feat: sync from pr72"

488be50

fuegoio suggested changes May 13, 2024

View reviewed changes

JamieLee0510 added 4 commits May 14, 2024 10:58

feat: private fetch and related test case update

caf2fbc

feat: tsup sourcemap

6cdfd5d

feat: js-doc catch up

7167a6a

refactor: get rid of babel for jest, by using in config

5b7ea12

sublimator reviewed May 14, 2024

View reviewed changes

fix: sync wording and class attribute variables

24e6edf

sublimator reviewed May 14, 2024

View reviewed changes

src/utils/helper.ts Outdated

});

return controller.signal;

}

Copy link

Contributor

sublimator May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem it wants you to add an extra line at the end

sublimator reviewed May 14, 2024

View reviewed changes

tests/test-utils.ts

* @return {Object}

*/

export function mockEmbeddingRequest() {

// TODO:

Copy link

Contributor

sublimator May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undone TODO

sublimator reviewed May 14, 2024

View reviewed changes

sublimator reviewed May 15, 2024

View reviewed changes

feat: sync main branch & returns jsdoc

48b0bfd

JamieLee0510 requested a review from fuegoio May 20, 2024 09:56

jwerre mentioned this pull request Jun 7, 2024

Remove node-fetch as a dependency #83

Closed

sync: file & job object

097fc2a

JamieLee0510 added 2 commits June 9, 2024 09:04

lint: ts file eslint & fix

052c31d

lint: update .eslintrc file

b486931

GaspardBT closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ts tsup #73

Feature/ts tsup #73

JamieLee0510 commented May 8, 2024

JamieLee0510 commented May 13, 2024

fuegoio left a comment

fuegoio May 13, 2024

fuegoio May 13, 2024

fuegoio May 13, 2024

JamieLee0510 May 14, 2024

fuegoio May 13, 2024

JamieLee0510 May 14, 2024

JamieLee0510 commented May 14, 2024 •

edited

Loading

sublimator May 14, 2024

sublimator May 14, 2024

sublimator May 14, 2024

JamieLee0510 May 14, 2024

sublimator May 14, 2024

JamieLee0510 commented May 14, 2024

sublimator commented May 14, 2024 via email

sublimator commented May 14, 2024 via email

sublimator May 14, 2024

sublimator May 14, 2024

sublimator May 14, 2024

sublimator May 14, 2024

sublimator May 14, 2024

sublimator May 14, 2024

JamieLee0510 May 14, 2024

sublimator May 15, 2024

sublimator May 14, 2024

sublimator May 15, 2024

sublimator May 15, 2024

JamieLee0510 May 16, 2024

JamieLee0510 commented May 16, 2024

jwerre commented Jun 7, 2024 •

edited

Loading

JamieLee0510 commented Jun 8, 2024

JamieLee0510 commented Jun 8, 2024

GaspardBT commented Oct 10, 2024

Feature/ts tsup #73

Feature/ts tsup #73

Conversation

JamieLee0510 commented May 8, 2024

JamieLee0510 commented May 13, 2024

fuegoio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieLee0510 commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieLee0510 commented May 14, 2024

sublimator commented May 14, 2024 via email

sublimator commented May 14, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieLee0510 commented May 16, 2024

jwerre commented Jun 7, 2024 • edited Loading

JamieLee0510 commented Jun 8, 2024

JamieLee0510 commented Jun 8, 2024

GaspardBT commented Oct 10, 2024

JamieLee0510 commented May 14, 2024 •

edited

Loading

jwerre commented Jun 7, 2024 •

edited

Loading