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

V001 rewrite ts microbundle #41

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

pBread
Copy link

@pBread pBread commented Jan 16, 2024

Hi All,

I was playing around with the JS client and ran into a few environment compatibility issues, e.g. a conflict to do w/JS’s lovely module declaration rules. I ended up refactoring most of the client. It's obviously a larger pull request than I'd normally do, but this is actually the easiest way to resolve the problem.

There are no breaking changes: the client methods work as before, tests were ported, and the examples are all working (except for the React example, but that's unrelated).

You should be able to clearly follow the git commit history. I went through it step by step and as logically as possible.

Here are the major enhancements:
Native Typescript Support: It’s quite tedious to make a native JS compatible with TS. Going from TS to JS is easier and you benefit from type-safety. This client should come with very robust typings now.

Microbundle: Microbundle is wonderful. There almost zero configuration and it takes care of polyfill, minification, cross environment compatibility, TS declaration generation, etc. etc.

Microbundle solves the original problem I ran into. And, it will immediately add functionality, such as producing the UMD files necessary for browser <script> support.

Replaced node-fetch w/isomorphic-fetch: isomorphic-fetch is just a wrapper around node-fetch that solves the globalThis.fetch compatibility problem.

Also, jest-fetch-mock is used for mocking fetch.

Added Versioned Facade: Mistral's API is obviously very new. I expect the routes and parameters will change drastically in the coming months. I've added a versioned facade in front of the root-level methods which will allow the client to add temporary fixes to breaking changes, version itself, etc.

Finally, I also fixed some of the JSDocs comments and a few minor things.

created empty client.ts. will slowly migrate changes to that file
added microbundle config to package.json
Changed "APIError" => "ClientError" in case errors w/client itself and not api

Added prototype continuity to ensure instanceOf works properly
…ns it's a client factory or constructor, instead of a class.
added private keyword to sensitive data, i.e. apiKey should not be public

Defined MistralClientConfig as an object. It's cleaner

Rewrote JSDocs comments for constructor
offloaded streaming logic to utility function
I realized this is going to have to be a public method, at least for now, to use typescript's type inference capabilities
This is more aligned with the route structure. The root-level methods will call these methods.
added generic typing to _request
type declarations will automatically be generated by microbundle
this will make environment variable loading more consistent across dev environments
The server is using the Server-Sent Events (SSE) format. Right now, the client just passes the raw string but that could be handled in the client, as well. That'll be a separate issue
Moved current tests to new file so I can systematically go through the tests and ensure non are lost
I was running into issues where jest wasn't defined. Moving it to __test__ directory seemed to resolve the issue.

I also added jest-fetch-mock, which resolved some mocking issues. I might remove it.
Need to work through additional tests and streaming is giving me an issue. Will dig into it shortly
Had to make the util function handleStreamResponse a class method so it could be mocked
@Bam4d
Copy link
Collaborator

Bam4d commented Jan 16, 2024

Hey, This looks great. Thanks for you work!

I think for such a big change it would be good to have this in your own repository such as pBread/client-ts.

We can point people to your version on our third party libs page also.

Thanks!

@pBread
Copy link
Author

pBread commented Jan 16, 2024

I’d like to contribute to this project. Mistral’s API is going to change and the core project will have more support.

The problem is there are several important building blocks missing from the current implementation. Notably, a build system and native Typescript support. Implementing those require a lot of re-write and directory structure changes.

In either case, I’ll leave the branch as it is. You can reference it when you want to tackle those features.

In the meanwhile, there are some changes I can port into smaller pull requests.

@sublimator sublimator mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants