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

npm run typegen - generate TypeScript declaration files #26

Closed
wants to merge 6 commits into from

Conversation

kungfooman
Copy link
Contributor

Fixes #25

This is a basic type setup, which enables type-checking/Intellisense aswell:

image

I just added two example JSDoc comments, to display how it works.

Since @chelouche9 asked for it, I would like to assign you to work out more types. Are you capable of doing so?

Whenever you have added more types, you can run:

npm run typegen

And commit both the JSDoc comments and the dist/types changes, so we have synchronized types.

@xenova
Copy link
Collaborator

xenova commented Mar 18, 2023

Wow this is great! Thanks so much! I'll wait for @chelouche9 to respond before proceeding to merge into main.

Excuse the potentially silly question (I am not too familiar with typescript haha), but does it have to be a dependency on the library itself, or can it be made into a dev dependency?

I could also try to add types while I write the code. I assume I just follow what you did and add annotations to the methods?

@chelouche9
Copy link
Contributor

@kungfooman sure, I would be happy to add more types to the project. Just to make sure I understand, I need to add new JSDoc comments, then run the typegen command and the declaration files will be created?

@xenova anything I need to know about running the project, tests, etc..?

@xenova
Copy link
Collaborator

xenova commented Mar 18, 2023

@xenova anything I need to know about running the project, tests, etc..?

My "test-suite" (if you can even call it that) is very simple. I made it before actually creating the npm package, so it is still just an ugly script that I run with npm run test (https://github.com/xenova/transformers.js/blob/main/tests/index.js)

To run the tests with local models (ofc much faster for development), you need to have the models stored in the models folder (https://github.com/xenova/transformers.js/tree/main/models) with the same structure as the HF repo I use to store the remote models: https://huggingface.co/Xenova/transformers.js. For example, you will have folders like this:
./models/onnx/quantized/openai/whisper-tiny.en/speech2seq-lm-with-past

Alternatively, you can disable using local models:
https://github.com/xenova/transformers.js/blob/4ad2581604f2b8da37e8842f9453d02ae1c4f5e4/tests/index.js#L14

but since I haven't implemented local caching, you will end up downloading the model each time you run the tests (not ideal)


Other than that, you should just be able to clone the github repo and it should work fine.

@chelouche9
Copy link
Contributor

@xenova great, I will start working on that once you merge :)

@xenova
Copy link
Collaborator

xenova commented Mar 18, 2023

@xenova great, I will start working on that once you merge :)

Actually, do you mind working on this branch (or a fork of it)? Then I can merge it when it's all ready?

@chelouche9
Copy link
Contributor

Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.

@xenova
Copy link
Collaborator

xenova commented Mar 18, 2023

Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.

Great! The library is expanding extremely quickly, so there might be quite a lot of breaking changes, but many functions should remain the same (and I will take responsibility for managing any conflicts before merging into main)

@kungfooman
Copy link
Contributor Author

Excuse the potentially silly question (I am not too familiar with typescript haha), but does it have to be a dependency on the library itself, or can it be made into a dev dependency?

Sorry, my bad, TypeScript is of course a dev dependency only. I fixed the package.json and package-lock.json via npm install --save-dev typescript.

I could also try to add types while I write the code. I assume I just follow what you did and add annotations to the methods?

Right, it's rather simple but some types can become complicated aswell. JSDoc is pretty powerful, but if I don't know the syntax, I've had good experiences with just asking OpenAI 😅

@kungfooman sure, I would be happy to add more types to the project. Just to make sure I understand, I need to add new JSDoc comments, then run the typegen command and the declaration files will be created?

That's right, everything is quite straight-forward with types. 👍

Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.

I have the same sentiment, typing is something that developes over time. Usually when I'm stuck with some code that I cannot understand so far, I add types as a mental prop e.g.

It doesn't take too much time, in VSCode you just move the cursor over e.g. a method, type /**[enter] and it will auto-generate the entire @param template.

@chelouche9
Copy link
Contributor

Right, it's rather simple but some types can become complicated aswell. JSDoc is pretty powerful, but if I don't know the syntax, I've had good experiences with just asking OpenAI 😅

I am using it as well to do all the heavy lifting 😝

Sorry, my bad, TypeScript is of course a dev dependency only. I fixed the package.json and package-lock.json via npm install --save-dev typescript.

I added it to my PR. I cannot push to your branch @kungfooman. If you guys have a better idea to manage those branhes, let me know.

@chelouche9
Copy link
Contributor

#28

@kungfooman
Copy link
Contributor Author

kungfooman commented Mar 19, 2023

I added it to my PR. I cannot push to your branch @kungfooman. If you guys have a better idea to manage those branhes, let me know.

Thank you for being pro-active! I added you both as collaborators on my fork, so that should work, once you accepted the invite.

I hope you know how to add remotes and pull/push to them

Another way is of course to wait until this is merged and then do PR's as usually. But in the mean time, this should be possible now.

@chelouche9
Copy link
Contributor

@kungfooman I applied all the changes on my fork already. I will keep doing it from there :)

@kungfooman
Copy link
Contributor Author

@kungfooman I applied all the changes on my fork already. I will keep doing it from there :)

Nice, so either:

  1. This will be merged first and you can merge main branch
  2. Your PR will be merged directly

I have no hard feelings for either, I'm glad about your support 💪 👍

Once a typing base exists, it will be easier to make more fine-tuned type fixes here and there in smaller/more-manageable PR's

@chelouche9
Copy link
Contributor

@kungfooman are you actively work on this branch?
I though for some reason you have made the setup and stopped

@kungfooman
Copy link
Contributor Author

@kungfooman are you actively work on this branch?

I just keep it up to date basically, so either way (1) or (2) should work out nicely

@chelouche9
Copy link
Contributor

@kungfooman If you don't mind, I will keep it on my fork. I have already set it up and everything is working fine :)
Nonetheless, maybe @xenova will decide to merge this PR first. I will make sure to update and fix conflicts.

@kungfooman
Copy link
Contributor Author

continuation in #28

@kungfooman kungfooman closed this Mar 20, 2023
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.

[Feature request] Typescript Support
3 participants