Skip to content

Typescript #1145

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

Closed
wants to merge 3 commits into from
Closed

Typescript #1145

wants to merge 3 commits into from

Conversation

tsekiguchi
Copy link

Hi @xenova and team,

First off, I want to say how much I’ve been enjoying Transformers.js! I’ve been using it for a couple of months now, and it’s truly an incredible library. Thank you for all the work you’ve put into it!

That said, I’ve encountered some challenges related to types, auto-complete, and error handling while working with the library. To help address these and improve the overall developer experience, I’d love to embark on a bigger project: rewriting Transformers.js in TypeScript.

The benefits of this rewrite would be substantial—it would make the library even more robust while greatly enhancing DX. It also presents a great opportunity to revisit some core code, tackle any outstanding TODOs, and adopt modern JavaScript patterns, such as using typed objects instead of maps/dicts where appropriate.

To get things rolling, I’ve started with a few files in the utils directory, but I’d like to submit my work on RawImage image.ts first to ensure the approach aligns with everyone’s expectations before proceeding further.

Additionally, I’ve implemented consistent error handling using try/catch. Errors will still throw as before but will now pass through console.error to log them first. This addresses a gap where some existing error statements weren’t getting logged, leading to crashes without feedback.

Importantly, the goal of this TypeScript rewrite is not to break anything. We’re maintaining the existing methods and inputs unless absolutely necessary, so nothing currently in production should be affected.

Thanks, and looking forward to making this library even better!

@kungfooman
Copy link
Contributor

That said, I’ve encountered some challenges related to types, auto-complete, and error handling while working with the library.

Utter fanboy ignorance. You will destroy the ease you can test PR's with right now with nothing but ESM and an importmap, hence overcomplicating everything through a chain of useless intermediary tools like TSC and putting the entire project at the mercy of "I hope the build chain works with my setup". I've tested so many TS projects over time and if they are written in TS, it's highly likely the build process broke somewhere.

Just fix the JSDoc types, if there are some issues. Also, we had this discussion several times...

@BritishWerewolf
Copy link
Contributor

BritishWerewolf commented Jan 14, 2025

As stated above, there is no reason to rewrite this in TypeScript, because this library already has type safety through JSDoc.
JSDoc is supported Out the Box with VS Code - and I imagine that other IDEs will have support too.

Your time would be better spent improving the current documentation comments to improve the type safety.

@tsekiguchi
Copy link
Author

Okayyyyy. I hear you. 😮‍💨

The goal here isn’t to debate JSDoc versus TypeScript. At the end of the day, we can all agree that types are really helpful for improving the developer experience – ensuring that developers who are implementing this library know what information to input, and what to expect when it comes out. For many of us, TypeScript is simply one tool that we use to accomplish this. If specific decisions have already been made regarding TS, then let’s visibly document them so that others can understand the reasoning and we can avoid revisiting.

Don't get me wrong - there's a ton of great JSDoc annotations already, and I appreciate that! But there’s some specific modules that I found difficult to reckon with. For example, when trying to call on processor, there is no indication of text, image, or config, which was problematic when I was trying to get it to work with a CLIP model that only wanted an image input.
Screenshot 2025-01-14 at 12 45 07

For comparison, looking at CLIPProcessor in the python transformers library, the __call__ method is allowed to specify inputs, which is incredibly helpful. It also has plenty of information about the method
Screenshot 2025-01-14 at 12 50 35

I believe this stems from using the Callable structure. Is there a way to have a JS equivalent in terms of typing and information on the callable? Maybe there's additional methods that could be added/exposed that provide an alternate way in, like processText, processImage, and processTextAndImage? Would love to hear if maybe this has already been explored, or if it's a problem that could be worth solving.

Ultimately, I genuinely just want to help make this library a little more approachable and thought I saw a way I could help out. I understand it’s not how you would approach it. But honestly, for as kind as xenova has been in all his communications, it’s a bummer to be on the receiving end of comments like yours. There’s zero reason why we can’t talk about a problem and be nice to each other.

@tsekiguchi tsekiguchi closed this Jan 15, 2025
@BritishWerewolf
Copy link
Contributor

BritishWerewolf commented Jan 15, 2025

To clarify, I never meant to offend with my comments. I just didn't want you to spend anymore time translating anything else into TypeScript when there is already a type system in place.


Without digging into the code myself, I'm not 100% sure why the IDE doesn't work properly for Processors - I am sure there are comments.
Perhaps that is something you can look into and you can create a PR for that?

The work you did here is fantastic, and your comments are thorough and in-depth; I am confident in saying that your pull requests will be greatly appreciated again in future.
Please don't let this dissuade you from working on the project in future!

@tsekiguchi
Copy link
Author

Thank you! I really do appreciate that. To be clear it wasn't your comment that I was referencing.

I'll do some more digging and see if there isn't a solution for JSDoc, and will submit a PR if I find anything :)

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.

3 participants