FNNDSC / ami

AMI Medical Imaging (AMI) JS ToolKit
https://fnndsc.github.io/ami/#viewers_upload
MIT License
717 stars 213 forks source link

[refactoring] Typescript definitions #152

Open Flos opened 7 years ago

Flos commented 7 years ago

It would be nice to have typescript definitions available.

NicolasRannou commented 7 years ago

Agreed - do you have some experience with it @Flos ? I'm new to typescript so it may take some time!

Flos commented 7 years ago

Im also quit new to it, but I started to add types right away.

Its more ore less taking the available javascript files, renaming them to d.ts and only keep the function headers. Then adding where possible the right types. I will share it when im done with the first iteration, AMI.js has already quite some code, I hope im done with it end of the this week.

Flos commented 7 years ago

Im now adding typescript definitions so they can be imported later as an npm package, but they have to be updated additionally all the time, so they stay in sync with the current version of ami.js.

Since the project is still in an "early stage" (version number), and typescript will make the code cleaner, and developing easier. How about switching completely to typescript? I know its quite some work, but I think its worth it.

NicolasRannou commented 7 years ago

What do you mean by the definitions have to be updated all the time? Update definitions each time we add a new public method right?

I'd rather stick with "vanilla" javascript as it is a standard for the moment but we may move some point.

In your experience, it is fine to import external JS libraries while working with TypeScript? Which would be the steps to switch to TypeScript? What is the main advantage in your opinion? (strong typing? cleaner? less code? faster? etc.)

Thanks @Flos, looking forward to your feedback!

Flos commented 7 years ago

Update definitions each time we add a new public method right?

or refactoring, changing namespace,... renaming function, classes, ...

In your experience, it is fine to import external JS libraries while working with TypeScript?

When its needed, I don't see a reason why not.

What is the main advantage in your opinion

The types I create will help to use this library, without the need to look into the source code or read the documentation to extensive. If this would be a typescript project, also everyone developing the project would benefit directly by a nice support from the editor. It will always know all available function of the objects/scopes you use. The editor and transpiler will warn or error if something is wrong (parameters, return value). Right now some files have documentation about the expected input and output parameters, but thats inconsistent and still just a comment in the .js file, these information will just move in the typescript files and will support everyone actively. A detailed look into the source files is not needed then. I would expect that the transpiled and bundled javascript file will be faster.

Which would be the steps to switch to TypeScript?

I see there are a few tickets, as packaging, library build for npm, THREE not global etc. Those could be resolved in one big step to typescript.

  1. Rename files from ".js" to ".ts"
  2. Update package.json add devDependencies
    "@types/three": "^0.84.12", 
    "typescript": "^2.2.2",

    update scripts to transpile on build "build": "tsc"

  3. create tsconfig.json with something like
{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs",
    "sourceMap": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "removeComments": true,
    "noLib": false,
    "noImplicitAny": false,
    "declaration": true,
    "outDir": "lib",
    "noUnusedLocals": true,
    "noUnusedParameters": true
  }
}
  1. Refactor, fix issues, add types where useful

Roughly thats it.

NicolasRannou commented 6 years ago

@Flos Did you look into that already? The new webpack build system may (or not :/) help Thanks!

NicolasRannou commented 6 years ago

FYI I think it may be time for that. Will need to see how to make it work well with webpack and tests!