Consider supporting Typescript

oveddan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

shiffman commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

any updates?

joeyklee commented 4 years ago

@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 commented 4 years ago

news?

cyberprodigy commented 4 years ago

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

joeyklee commented 4 years ago

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 https://github.com/ml5js/ml5-library/issues/253#issuecomment-503316037

Nezteb commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Has there been any movement on this?

jha-prateek commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/ml5js/ml5-library/pull/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 commented 1 year ago

d.ts declaration file please!

PedroEliasCS commented 1 year ago

Any news?

ml5js / ml5-library

Consider supporting Typescript #253