TypeScript Code Style and workflow

[afrokick]

The possibility of TypeScript in Meteor become a reality and official supported. It was discussed here Official TypeScript compiler coming to Meteor

We’re starting to convert some .js files to .ts https://github.com/meteor/meteor/labels/Project%3ATypeScript-conversion

But I think we need to accept some code style for TS at first.

@benjamn Do you have idea about it? may be some template?

UPDATED 17 Jul
Q https://github.com/meteor/meteor/pull/10614#issue-296046123 @menewman

1. Is the correct way to check for TS errors in this project just to open the project in VSCode and see if it gets angry? (That’s what I did and it seemed to work… okay.)
2. For whatever reason, TypeScript lets me get away with code like const pipe = require("multipipe"); but not import pipe from "multipipe"; , because for the latter version it wants me to install the type defs for that package via NPM. I have no idea how to add new NPM dependencies to meteor/tools and I’m not sure whether it’s advised/allowed even for TS types. What do you recommend we do in cases like that?
3. I submitted this into the release-1.8.2 branch because that’s where your “convert files.ts” PR was targeted, but I’m not sure if this was the right thing to do, since the Contributing guides seem to encourage submitting into devel . However, devel doesn’t seem to have all of your TS-related changes from release-1.8.2 .

A https://github.com/meteor/meteor/pull/10614#pullrequestreview-260210683 @benjamn

1. Yes, I think we should use external tools (especially VSCode) for TypeScript type checking, and let meteor-babel simply do the compilation. That said, you can also run meteor npx tsc from within the meteor/tools directory, which is something we should do as part of our Circle CI tests (edit: implemented by 06b5f32).
2. Totally fine to keep it like this (using require ) for now. At some point we’ll do a pass to convert everything to import s and make sure the appropriate @types/… packages are installed. That’s the kind of thing that should be easy to automate, so I don’t want to waste human time.
   Technically, we will need to add @types/split2 to dev-bundle-tool-package.js and rebuild the dev bundle. You can simulate that (without rebuilding the dev bundle) by running meteor npm install -g @types/split2 .
3. Yep, the release-1.8.2 branch is the only place (besides release-1.9 , which is still experimental) where TypeScript is supported.

Q @types https://github.com/meteor/meteor/pull/10633 @nicusor

Add @types/semver@5.4.0 to both dev-bundle-tools-package.js and dev-bundle-server-package.js
@types/semver is important because multiple files that should be transformed to TypeScript require it (e.g. utils/utils.js ).
Regarding the versioning mismatch. There shouldn’t be any problems since the only difference between 5.4.0 and 5.4.1 is the addition of a try...catch statement in commit semver/7be83d15dcff73f6, so the interface wasn’t changed.

A https://github.com/meteor/meteor/pull/10633#pullrequestreview-262486357 @benjamn

… This will require rebuilding the dev bundle after I merge it into release-1.8.2, though you can do that locally by running

scripts/generate-dev-bundle.sh
rm -rf dev_bundle
meteor --help # downloads the new dev bundle

Summary

• Use release-1.8.2 branch to merge your pull request
• Run cd path/to/meteor/tools && ../meteor npx tsc to compile TS code in tools
• Install global npm i -g @types/XXX if you need it.
  Also it can be a part of dev-bundle in scripts/dev-bundle-server-package.js, scripts/dev-bundle-tool-package.js

[trusktr]

You could strick Prettier into your project. I use that for code style for HTML, CSS, JS, TS, etc.

[afrokick]

Of course, Prettier is very cool instrument.

But I mean TS very flexible language, and, e.g. you can define class field explicitly or via Parameter properties. So we need some paper to describe such cases, basic rules etc.

[afrokick]

@menewman @nicusor I mentioned you in the first post. Hope you’r interested in it.

I updated first post with some answers from @benjamn.

Some questions:

• Do we need to update .eslintignore to ignore .ts files?
• When we should use old require instead of adding @types to dev-bundle and use new import?

And suggestion:

• We can add .vscode folder with basic settings to /tools.

[robfallows]

We can add .vscode folder with basic settings to /tools

for this. That would be a great way of ensuring consistency.

[nicusor]

I think it would be important to use both eslint and prettier.
Formatting specifically is pain point for me. During the conversion of several files I’ve tried my best to make my code look like the one I was seeing in the rest of the codebase. Having consistent, enforced formatting rules would aleviate that.

Regarding eslint. At the project I’m currently working we’re using prettier-eslint (although we’re using TypeScript by embedding it in JsDoc using @ts-check). I’m not very knowledgeable about the current state of integrating tslint with eslint, but as I know, eslint works well for TS.
We’ve adopted the AirBnb style guide, but also made some tweaks because some rules seemed erroneous, unnecessary or could be warnings instead of errors. Since this is an open-source project I would gladly adhere to stricter eslint rules.

[nicusor]

Another things I wanted to mention. We’re using React so to enforce that state and props are immutable we’re using DeepReadonly from ts-essentials because TS by default has just Readonly<T> and ReadonlyArray<T>.
This is cool because you can make TypeScript check that parameters are not mutated inside functions (where that is the case).

[menewman]

• When we should use old require instead of adding @types to dev-bundle and use new import?

Thanks @afrokick!

Regarding whether to use require vs. adding @types to the dev-bundle, Ben pretty much answered that for me here:

Convert eachline.js to eachline.ts

Totally fine to keep it like this (using require ) for now. At some point we’ll do a pass to convert everything to import s and make sure the appropriate @types/… packages are installed. That’s the kind of thing that should be easy to automate, so I don’t want to waste human time.

I know you already linked that comment/answer, but I think Ben is just saying “keep using require if you have to and don’t worry about it.”