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

Consider supporting Typescript #253

Open
oveddan opened this issue Dec 30, 2018 · 29 comments
Open

Consider supporting Typescript #253

oveddan opened this issue Dec 30, 2018 · 29 comments

Comments

@oveddan
Copy link
Contributor

oveddan commented Dec 30, 2018

I want to open the discussion here around allowing models to be developed in typescript in ml5. I feel like using Typescript would bring many advantages.

  • We are dealing often with new libraries to import that in general are made in typescript. These libraries have a lot of unknowns; being able to have autosuggest for all of the possible library methods when we don't know the full api would make things much easier.
  • We would know right away when we have typos or use a tensorflow.js or third party method incorrectly, saving a lot of debug time.
  • When new developers come into contributing ml5, they can more quickly get accustomed to what all the methods are by just pressing a dot when typing in the IDE and having all the options appear right away. If we encourage descriptions on the method signatures, these would show up too. They'd also be less scared of mistyping something and breaking things because typescript lets you know right away if there was a mistake.
  • Typescript annotations make reading code a lot easier, especially when there are tons of different internal types going around.

I know there is the argument that we want to make ml5 easy for people new to coding to contribute to, and that by throwing in Typescript they'd have to learn something new on top of javascript and it would potentially hinder them from contributing. I actually would think in the long term it would make it easier for them to contribute and make it faster for them to fix bugs and add features because of the points stated above. They should be able to learn the basics of Typescript by seeing existing code in the repo and reading some intro documentation (it's very well documented) and once they get over that hump with VSCode's awesome typescript integration they'd have a much better experience coding with the repo.

I think a basic start would be to allow (but not require) new models to be made in Typescript. This could be a trial.

What are your thoughts?

@huan
Copy link

huan commented Dec 30, 2018

I just came here and saw this awesome ml5js module for js users, it's really fantastic!

And I would like to vote YES for writing code in TypeScript because it will really help us to save lots of time to maintain the code with higher quality.

Here's a wonderful TypeScript talk from Anders Hejlsberg at here

@cvalenzuela
Copy link
Member

We have discussed this a few times, but given the size of the project now, I think this is a very reasonable change to explore and possible make. Will take some time to update the source but might be worth it. Perhaps we can discuss this in more detailed on our January meetup? @shiffman, what are your thoughts on this?

@nathanvogel
Copy link

nathanvogel commented Jan 2, 2019

I've never really used Typescript, but when I quickly hacked ml5 to have the classifier support Canvas input, understanding the supported types of the arguments and variables was my main struggle.

@huan
Copy link

huan commented Jan 3, 2019

@nathanvogel Yes, that's exactly why we need TypeScript!

@shiffman
Copy link
Member

shiffman commented Jan 14, 2019

I am very happy to revisit this discussion! My thinking has been to stay with JavaScript in terms of the project being easier to contribute to for beginner coders and alignment with p5.js (I like opening up the source code in class to show how the library works and have it be recognizable for students). @oveddan brings up a lot of good points. Let's discuss next week during our meetings at ITP, I'll be sending out some e-mails about that soon!

@shiffman
Copy link
Member

This came up a bit in our discussions at ITP yesterday. One idea proposed was to do this incrementally and maybe use typescript for new models/classes we are adding to ml5 and see how that goes. Would that be possible? Keeping the existing code base as is (for now) and adding a new class with typescript as a trial run?

@shiffman
Copy link
Member

Here's a nice quick intro to TypeScript for those so interested! (Like me)

https://dev.to/robertcoopercode/get-started-with-typescript-in-2019-6hd

@oveddan
Copy link
Contributor Author

oveddan commented Jan 27, 2019

Hey just got back in town. I think trying it for one or two new models makes sense. It should definitely be possible to do with javascript and typescript side by side. I can take a stab at this for the BodyPix implementation.

@mateja176
Copy link

Right now when you attempt to install type definitions for ml5 like so

npm install --save-dev @types/ml5 

you are met with the following error:

npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@types%2fml-5 - Not found
npm ERR! 404 
npm ERR! 404  '@types/ml-5@latest' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/username/.npm/_logs/2019-06-18T21_08_22_838Z-debug.log

As a deeply invested typescript user the only thing I lacked was type definitions for the library. However, based on a minimal introductory project, ml5 has held up to, if not surpassed, my expectations. I like it so much it fact ( and the whole concept of a high level, accessible machine learning library ) that I would love to contribute.

Following is a list of helpful resources:

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html
https://github.com/DefinitelyTyped/DefinitelyTyped#how-can-i-contribute
http://definitelytyped.org/guides/contributing.html

@dikarel
Copy link

dikarel commented Jul 19, 2019

Hi, I like this library and I'm in support of having a @types/ml5 type definition library. In my experience, autocomplete has helped me learn quickly as a beginner -- it lets me explore possibilities and without constantly stumbling over trivial mistakes.

Has anyone started working on this? I'm happy to take a quick stab otherwise.
In case this approach conflicts with existing plans, let me know!

EDIT: A quick draft that I did

@kresli
Copy link

kresli commented Nov 14, 2019

any updates?

@joeyklee
Copy link
Contributor

@kresli - Thanks for checking in. It definitely is helpful to help gauge interest in certain features.
No updates at the moment, but this is something that is on our minds as a discussion point. If anything changes, we'll be sure to ping this issue!

@gurachan
Copy link

news?

@cyberprodigy
Copy link

Is it done? Please update the thread if it's done.

@joeyklee
Copy link
Contributor

joeyklee commented Feb 23, 2020

Hi All,
So far there are not updates on this request. As a quick note: Typescript is very cool and it is certainly a feature that would help in development. However, many of our contributors and maintainers are not (yet) Typescript users. So far our stance has been to keep the door as open as possible for those contributors and reduce the barriers to entry.

We're certainly excited about Typescript and the potential for more maintainable and predictable code behavior, but our priority is to focus on creating less entry barriers to contribution.

If there's particular motivation, please feel free to suggest or point to resources for ways we might integrate Typescript in a nice way! Thanks!

Making a note of the helpful resources from @mateja176 #253 (comment)

@Nezteb
Copy link

Nezteb commented Feb 24, 2020

I'd suggest not writing (or re-writing) all/any of ml5js in TypeScript, but maintaining a separate list of type definitions as part of DefinitelyTyped as @dikarel suggested. Their proof of concept is a great start!

As an example, P5 itself already has published TypeScript type definitions (just type "P5" into the search bar).

@joeyklee
Copy link
Contributor

@Nezteb - Ah brilliant. This makes perfect sense + very cool.

@bomanimc - Let's make a note of this as an action item.

Thanks all! Let's leave this issue open while we work to add these changes in. 🙏

@NullVoxPopuli
Copy link

Has there been any movement on this?

@jha-prateek
Copy link

jha-prateek commented Aug 9, 2020

Hi, I found a way to use ml5 with Typescript using few simple tricks. You may call it just a workaround till we have support for Typescript. I have a written a blog post on this. Check it out

@iam-yan
Copy link

iam-yan commented Sep 3, 2020

@joeyklee

but our priority is to focus on creating less entry barriers to contribution.

🤔 This is not a problem I think. What needed is not a ts rebuild, but an officially maintained type package. Just like the approach @dikarel suggested.

@koji
Copy link

koji commented Sep 3, 2020

Maybe trying ts-migrate(https://github.com/airbnb/ts-migrate)?
Just tried that with ml5-library. Got many errors lol

ex
https://github.com/ml5js/ml5-library/blob/development/src/Pix2pix/index.js#L62

passing 3 parameters even though the function requires 2 parameters.
https://github.com/ml5js/ml5-library/blob/development/src/Pix2pix/index.js#L141

The migration could be helpful to detect unknown issues

@dejavu1987
Copy link

Hi, I like this library and I'm in support of having a @types/ml5 type definition library. In my experience, autocomplete has helped me learn quickly as a beginner -- it lets me explore possibilities and without constantly stumbling over trivial mistakes.

Has anyone started working on this? I'm happy to take a quick stab otherwise.
In case this approach conflicts with existing plans, let me know!

EDIT: A quick draft that I did

I have used the draft from @dikarel in my starter pack for ML5 with webpack and TS setup. https://github.com/dejavu1987/ml5-typescript-webpack

It is a nice way to start, will happily contribute.

@lindapaiste
Copy link
Contributor

I am interesting in writing types for this package. Would the maintainers consider switching the codebase over to Typescript, or should the types be managed separately as part of the DefinitelyTyped project?

I've created type definitions for a package on DefinitelyTyped before but that one was pretty easy as the type types were simple and well-defined. The advantage of writing Typescript into the code as opposed to adding on declarations from outside is that it allows me to see what the types are really supposed to be based on errors in the functions. There are some things in the JSDoc that need way more information in order to be useful Typescript. For example the PitchDetection takes an Object and a function which are extremely vague types. What are the properties of the object? What are the arguments and return type of the function? I have to really dig into the code anyways to figure that out so it's probably not extra work to convert the code to Typescript.

@lindapaiste
Copy link
Contributor

lindapaiste commented Apr 19, 2021

I converted one model (Sentiment) to see how easy or hard it would be. There are some things that would not pass Typescript checks in "strict" mode because there are no default values for the class properties (some would be trivial to set empty defaults for, this.model is tricky). If the loadModel() method throws an error then there will be runtime errors when calling predict() because this.model and other properties will not have been set and there are no conditional checks.

The part that I really cannot get my head around is something in the JSDoc which seems to contradict what I'm actually seeing in the code. It says that this.ready is a boolean, but assigns this.ready = callCallback(this.loadModel(modelName), callback);. So isn't this.ready a Promise which resolves to a Sentiment or to an error (type any)? I don't see how that possibly becomes a boolean.

Here's how I typed callCallback:

export type Callback<T> = (error?: any, result?: T) => void;

export default function callCallback<T>(promise: Promise<T>, callback: Callback<T>): Promise<T | any> {
  if (callback) {
    promise
      .then((result) => {
        callback(undefined, result);
        return result;
      })
      .catch((error) => {
        callback(error);
        return error;
      });
  }
  return promise;
}

@gurachan
Copy link

gurachan commented Nov 16, 2021

If you want to use anyway, just add at the top of the file declare let ml5: any;

do you even typescript xD what makes typescript beautiful is you can see all the types and the IntelliSense is superb looks clean and neat.

@azzazkhan
Copy link

If you cannot re-write the entire codebase in Typescript then please at least create separate type definitions (@types/ml5) for this library, it's tough to see the documentation for the parameters of each function and their return types.

@joeyklee
Copy link
Contributor

Hi @azzazkhan -- thanks! My thinking has definitely shifted on this over the last couple years and I'd be happy to support incrementally updating ml5 to TS. @lindapaiste has some work open for this #1388

If you have some time and could review some of the PRs related to the PR linked above, that'd definitely move us closer to making this happen! TY!

@RichardSneyd
Copy link

d.ts declaration file please!

@PedroEliasCS
Copy link

Any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests