TypeScript!

[Zarel]

Been meaning to have an issue about TypeScript. I personally really want some sort of type checking in the repo, also because IntelliSense makes coding a lot nicer on VS Code, but also because PS has had at least a few crashes that could have been caught by stricter type checking.

Supposedly, research says that even in the absence of good test coverage and/or TDD, strict typing rarely leads to fewer bugs. I don't normally like going against research, but either way it still has plenty of benefits.

This issue is mostly to see what other contributors think of TypeScript.

[punmechanic]

I tried this with pokemon showdown. Here's are my frustrations when trying to convert battle-engine.js:

https://twitter.com/ARedHerring_/status/833344587183828992 https://twitter.com/ARedHerring_/status/833330440811446273

TypeScript is great (Nearly every project I do in JS I use it these days), but it'll be hard to get working just due to how incorrect some assumptions are about types in that particular file.

That said, that may just be all the more reason to convert it.

Also: We'll (very likely) want to all sources files into a src/ folder if we do this, as TypeScript is a compiler and will want to output to a bin/ folder or it will emit outputs in-place which will make the root folder extremely bloated. I had issues with mock-fs-require-fix while trying to do this and all log paths broke etc. Getting tests running was difficult as well - Even when specifying --require ts-node/register to mocha, I still had issues with JavaScript tests trying to load TypeScript files.

Finally, my experience with TypeScript in industry is:

• It reduces cognitive load (less stuff you need to carry in your head at the same time)
• It reduces barrier to entry in a codebase (easier for new people to contribute)
• It helps you verify your assumptions are correct
• It does catch bugs (Do you have a link to that research? I know of several bugs in my time in industry that have been caused solely by typing errors)

There are a few places in battle-engine.js where a lot of assumptions are made about some types (like this.decisions - in one place it's a boolean, in another it's an array, and in another it's an array of move decisions - which themselves are an array. Some functions are returning T | boolean to indicate success/failure, etc.

Now, to humans, that might "make sense" because you know what state the Battle class is in, but you can't prove that to the compiler, so the compiler throws a hissy fit unless you come up with a super weird type signature to describe that scenario: boolean | Decision[] | MoveDecision[][].

I think types would help make the code make a lot more sense to a relative newcomer.

Particularly with --strictNullChecks enabled which will catch a lot of issues, I'm sure.

[Zarel]

I can't find where I read that static typing doesn't catch bugs, but I've definitely had an Amazon engineer claim it, as well as an article I read online. Both of which seemed to be intended to push me towards TDD.

[punmechanic]

Static typing won't catch business logic bugs, but it will catch bugs to do with not checking if something is null/undefined, highlight confusing business logic and so forth :smile:

[punmechanic]

based on the herpderp in #3272, I'll see if I can't get something done with TypeScript over the weekend.

[punmechanic]

@Zarel is there a chat room that I can communicate with you / other contributors about questions I have? I noticed that there's a Code/Tech room on PS, but I didn't know if that's the right one.

[AustinXII]

@danpantry try play.pokemonshowdown.com/dev

[Zarel]

Yeah, the Dev chatroom is listed in several places, including CONTRIBUTING.md which of course you read, right? ;) ;) ;)

[taylor1791]

One of my co-workers researched to see if unit testing obviates static typing. This was their Master's Thesis. The conclusion was that static typing would have caught bugs that made it into popular opens source python libraries. The thesis did use Haskell, which has a fundamentally different type system than TypeScript. The thesis can be read here.

This does go against many opinions on the internet and this should not be sufficient to make any decision, but I wanted to provide counter research.

[Zarel]

I feel like the biggest argument for TypeScript is that every time we have new OMs, they crash PS due to type errors that could very easily have been fixed... :|

[Morfent]

Poring over the handbook, I'm pretty impressed by what it offers. Anything stricter than JS' type system is godsend, let alone the sugar for features JS lacks from other languages, though I don't think TDD should be neglected since the extra insurance of more easily knowing your types are doesn't protect you from screwing up your types. I'm probably not saying anything that wasn't mentioned earlier, but regardless this is something that looks interesting

[Zarel]

@danpantry, anyway, my recent refactor to decisions should at least fix this problem:

https://twitter.com/ARedHerring_/status/833344587183828992

[Zarel]

Well, TypeScript 2.3 is a game-changer:

https://github.com/Microsoft/TypeScript/wiki/Type-Checking-JavaScript-Files

It's probably overall better for our use-case, too.

[juanpaucar]

@Zarel if you are planning to use ES2017 as output for TypeScript compilation we should make sure that the node version is the appropriate to support such features. I'm making a PR for this. Please check (http://node.green/)

My background has been mostly in Haskell, Ruby and TypeScript so in my short experience i'd say that: You can add tests to cover some cases, but you'll never get to cover all of the branches of possible scenarios (not even with mutation tests). But, if you have a good type system which can represent accurately your domain model then it should not allow an invalid representation of that model and therefore all cases would be already covered by your type system. Though, you can still add tests but mostly integration tests

That said, I'd love to help with moving the application to TypeScript. Is there already a plan? Could I propose one?

[Zarel]

PS's usual plan is support the latest Node LTS version (and avoid using any features not supported by the latest Node LTS version). Usually. If there's a more modern feature we really want, we'll write a polyfill, or as a really last resort, increase our required Node version beyond LTS (so far, this hasn't happened yet). async/await are nice, but we don't use Promises often enough to drop LTS support.

So, in that context, it's probably a bad idea to set TypeScript to ES2017. I'll change it to ES2016 since I believe that's the latest version currently supported by Node 6 LTS.

...actually, currently we use a polyfill for Object.values in a few places, so ES2017 might be necessary to make sure TypeScript doesn't complain about those uses.

[Zarel]

As for moving the application to TypeScript: Currently, the plan is to instead move it to TypeScript-jsdoc-compatible JavaScript. This will be easier on third-party server mod writers, and skip the need for a compile step.

[Zarel]

Thanks for the link to node.green by the way, it's looks really useful!

[punmechanic]

...actually, currently we use a polyfill for Object.values in a few places, so ES2017 might be necessary to make sure TypeScript doesn't complain about those uses.

You can include libraries by doing the following for any polyfills:

{
  "compilerOptions": {
    "lib": [
      "es2015",
      "es2017.asyncIterable" // as an example
    ]
  }
}

In general, polyfilling with TypeScript is a little messy, but it works

[Zarel]

Oh, thanks! That's really useful.

[Morfent]

We need to add style guidelines for how we write with Typescript. e.g.

class Foo {
    bar: string
    baz: number
    constructor(bar: string, baz: number) {
         this.bar = bar;
         this.baz = baz;
    }
}

class Foo {
     /**
      * @param {bar=} string
      * @param {baz=} number
      * @return Foo
      */
     constructor(bar, baz) {
         this.bar = bar;
         this.baz = baz;
     }
}

const fs = require('fs');
import fs = require('fs');
import 'fs';

type Foo = [key:string]NodeJS.Timer;
/* @type {{[key:string]NodeJS.Timer Foo}} */

I'm assuming we're going with comments for declaring types and keeping the way we were already dealing with modules, but I do prefer ES6 modules. Enums would be really nice to use as well

[Zarel]

The current plan is to use checkJs, which limits us to JavaScript.

The main style issue is that we probably want to use @param {string} [bar] rather than @param {string=} bar for optional arguments.

[Morfent]

How should arrow function parametres be typed when needed? I'm assuming we can't do it with (foo: type) => {} type syntax

[Zarel]

TypeScript syntax works inside @param {...} tags.

[Morfent]

It does, with the exception of callbacks like foo.on('bar', msg => { /* ... */ }); from what I find. The only workaround I've managed to find is defining the callback outside the function it's being passed to and using @param then, otherwise TypeScript ignores when I use that or @ts-ignore. This isn't a very big issue though, since JSDoc works fine for typing in every other case.

[punmechanic]

You could just use function statements in this example. It's a few more lines, but the stack trace would be nicer in the event of a failure anyway as you will retain the method name.

function doSomething(foo) {
  foo.on('bar', onBar)
  ......
  return foo

  function onBar() {
   ...
  }
}

[Morfent]

I think we need to add some rules to the style guide pertaining to Typescript, such as:

• types that can also be null: ?type or type??
• spacing for object types: {[k:string]:any}, {[k: string]: any} or { [k: string]: any }?
• arrays: any[], [any], or Array<any>?
• multidimensional arrays: any[][], [any][], [[any]], or Array<Array<any>>?
• functions: when would including the parametre and return values like in (any) => any be preferable to simply using Function, or vice versa?

As well as which JSDoc comments are acceptable;

• @document is not
• @param, @type, @typedef, @return are
• what about @class, @extends, @constructor, @property, @name?

[Zarel]

?type

{[k: string]: any}

any[]

any[][]

(any) => any

Ask when they come up.

[Morfent]

For style with nullable/possibly undefined types, I think it'd be better to write them as type?/type= instead of ?type. Typescript's picky about how those are written, and =type is not valid syntax for function parametres while type= is. Some more complex types require ? to be placed after the type name rather than before, but I don't have any specific examples on hand at the moment.

[Zarel]

I guess...

[Zarel]

TypeScript is currently partially implemented.

I'm still thinking about actually transitioning to .ts files. It would fix approximately half of our struggles with TypeScript. And I can make the launcher automatically compile them (incrementally, even! thanks to the incremental compiler I just implemented for the client). Our error messages would preserve the line number, and I could probably even silently rewrite the error messages to correspond to the original .ts file lines.

Thoughts?

[HoeenCoder]

One of the questions I have is how different is a .ts file from a .js file? Is it just JS with some add-ons and a build step? If its not really different I would be more comfortable with a swap, regardless I should look into this more when I get some time in a few days.

[Zarel]

It's a build step and makes writing TypeScript a lot easier, in particular, a not-null assertion is !, and casts and type definitions are also a lot more concise.

A few of the ugliest hacks in dev-tools/global.d.ts and dev-tools/globals.ts would no longer be necessary. A few other ugly hacks we use will probably also stop being necessary.

[Zarel]

On the other hand, we'll have to use type | null instead of type?, but that's a small price to pay.

[punmechanic]

Makes a lot more sense, too. type?, type | null and type | undefined are all different and this becomes very apparent when you write React and/or use discriminant unions

[scheibo]

Our error messages would preserve the line number, and I could probably even silently rewrite the error messages to correspond to the original .ts file lines.

This is pretty trivial with 'source mapping'. Simply adding require('source-map-support').install();. to the ./pokemon-showdown script or test runners etc should be enough. Its also supported by Chrome etc so you can debug the .ts code directly.

There's no work started on TypeScript migration yet, but the plan is pretty straightforward: just rename the files to .ts one at a time, and add a compiler to ./pokemon-showdown.

Originally posted by @Zarel in https://github.com/Zarel/Pokemon-Showdown/pull/5181#issuecomment-465403979

I'm happy to get the ball rolling on this, however, I'm running into problems that maybe you have the answer to?. I've converted projects to Typescript before (eg. I extracted and rewrote the damage calc in Typescript and hope to eventually upstream it after some polish), but I did that as one big chunk instead of piecemeal.

Say I take one file (sim/prng.js, because its short) and rewrite it in Typescript (a723e0a). I then run into problems because I want to make tsc emit code (currently our tsconfig.json has it set not to), but by default it will do it in the same place. However, this causes errors regarding the JS files:

error TS5055: Cannot write file 'Pokemon-Showdown/config/config.js' because it would overwrite input file.

...

error TS5055: Cannot write file 'Pokemon-Showdown/sim/side.js' because it would overwrite input file.

error TS5055: Cannot write file 'Pokemon-Showdown/sim/team-validator.js' because it would overwrite input file.

Found 147 errors.

You can configure tsc to ouput to an "outDir", say "./build/". However, it only does this to the files which we "include". Because only part of the project is typed, we now have half the JS in one tree and half in another. One solution would be to have a secondary step for the build to copy the compiled JS files out build and into the main source tree - I think this is what the client does (to get src/*.ts into lib/*.js)? This is kind of ugly IMO, with the build artifacts intermingling with source, but I guess the client sets precedent (I don't think its a problem with like object files in other languages, because one format is binary and one is text, that's not the case with 'compiled' Typescript). We could alternatively copy the source files into build and run ./pokemon-showdown against the build directory.

Apologies if I'm missing something obvious.

I'm still thinking about actually transitioning to .ts files. It would fix approximately half of our struggles with TypeScript. And I can make the launcher automatically compile them (incrementally, even! thanks to the incremental compiler I just implemented for the client).

Can you point me to this incremental compiler? Is it in build-tools and I'm just missing it?

[Zarel]

Hm, what's the fastest compiler that supports TypeScript and either source maps or preserving lines?

https://github.com/swc-project/swc is a recent one I saw, I don't see support for either feature, though.

I would probably have the compiler emit code to the same directory, at least until an entire directory is converted. After the entire directory's converted, then we can split up source and build directories.

[Zarel]

I also have questions about how much performance overhead source-map-support has. We use stack traces in prod, after all.

[scheibo]

Hm, what's the fastest compiler that supports TypeScript and either source maps or preserving lines? https://github.com/swc-project/swc is a recent one I saw, I don't see support for either feature, though.

By 'fastest' do you mean compile times, or the performance of the generated code? And do we care about the former - sure tsc is slow, but is ./pokemon-showdown startup time that concerning? (Or is this about tests? The npm run test script already includes tsc? Is this so npx mocha is fast? We could configure incremental compiles?)

EDIT: Looks like you're maybe talking about babel? To support older versions of node?

I would probably have the compiler emit code to the same directory, at least until an entire directory is converted. After the entire directory's converted, then we can split up source and build directories.

That's the problem - this doesn't seem to be an option for the project. Unless there's some magic configuration values I'm not aware of (I'll go comb through the tsconfig.json docs), we'll just get a bunch of TS5055 errors like I pasted above. I'm assuming you know something I don't, because you already did this on the client?

I also have questions about how much performance overhead source-map-support has. We use stack traces in prod, after all.

I could be mistaken, but I believe source maps work after the fact. ie/ you get a stack trace from prod and combine it with your source map locally to determine where the actual location is. Thus zero performance overhead outside of perhaps minor differences at compile time. I'll dig around and try to find links for it, but thats how it works internally at my company with other tools (I haven't used Typescript at work, but have used/worked on other transpilers).

[scheibo]

I'll go comb through the tsconfig.json docs.

Nothing jumps out at me from http://json.schemastore.org/tsconfig and https://www.typescriptlang.org/docs/handbook/compiler-options.html. Maybe we could use --outFile to put everything into a single file?

I'll dig around and try to find links for it.

https://www.html5rocks.com/en/tutorials/developertools/sourcemaps/ seems to be a pretty good explanation. I think the node-sourcemap-support package I linked probably would add overhead as it rewrites the stacktraces, but as the article outlines there are different alternatives which wouldnt impose that overhead at runtime.

[Zarel]

By 'fastest' do you mean compile times, or the performance of the generated code?

I mean compile times. Generated code should be roughly equivalent at this point.

And do we care about the former - sure tsc is slow, but is ./pokemon-showdown startup time that concerning?

Yes; fast feedback loops are important for quick development. Especially since there's no real tradeoff. I'm no fan of using tsc for compilation; it's not particularly featureful as a compiler, as you seem to be discovering.

That's the problem - this doesn't seem to be an option for the project. Unless there's some magic configuration values I'm not aware of (I'll go comb through the tsconfig.json docs), we'll just get a bunch of TS5055 errors like I pasted above.

Sounds like a good reason to use a different compiler. :p

Can you point me to this incremental compiler? Is it in build-tools and I'm just missing it?

https://github.com/babel/babel/pull/8877

In the client, it's just shoved into the repo:

https://github.com/Zarel/Pokemon-Showdown-Client/tree/master/build-tools/babel-cli

Anyway, the server doesn't need to compile down to ES3, so there's no need to screw around with Babel like this. I'd rather just use an off-the-shelf compiler designed for speed, like swc.

I'm assuming you know something I don't, because you already did this on the client?

The client puts TypeScript in src/ and JavaScript in js/, and sets the TypeScript outdir to js/. In hindsight, a similar trick might work here, too.

[Zarel]

The client puts TypeScript in src/ and JavaScript in js/, and sets the TypeScript outdir to js/. In hindsight, a similar trick might work here, too.

Oh, perhaps more importantly, the client uses globals for everything, there's no require/import paths to worry about.

[scheibo]

Yep. The paths all become bungled.

I'm also having trouble with hybrid the hybrid js/ts requires. I can do something like 043d4f8 for importing types, but I haven't figured out yet how to require sim/prng.ts in sim/battle.js for instance (where we need to actual constructor, not just the type).

My take from Googling is that theres not really a blessed path forward for an already jsdoc typed project to convert piecemeal (let alone a partially typed project like PS). It seems you can either type in jsdoc for js files or type in ts files, but mixing the two is not really that clearly supported.

[Zarel]

You might need to convert the entire sim/ directory at once. At least it's comparatively small.

[Zarel]

You can also remove --strict, so it's easier, and then slowly bring it back one file at a time.

[scheibo]

You might need to convert the entire sim/ directory at once. At least it's comparatively small.

This would only work if none of the other typed-JS code depends on things within sim/ (which they don't because they definitely want sim/dex.js). And even then I'm not sure it would work because I don't know about how clever Typescript is with ordering (will it know to compile and emit code from sim-ts/ to sim/ so that all the other JS requires work? I highly doubt it). I also think we'd need to convert sim/s dependencies (data/, lib/streams.js) for it to compile.

Even if this approach did work, it would still be forcing us to effectively be converting directories at a time, and while sim/ is relatively small, its not an ideal approach.

You can also remove --strict, so it's easier, and then slowly bring it back one file at a time.

Yeah, I kind of wanted to avoid that. It seems really garbage to have to basically lose the benefits of type checking during the period of migration between typed-.js and .ts. :(

What do you think of using --outFile to merge all of the source files (.js and .ts into a single file)? It might also be a non-option because of the ".js doesn't seem to be able to require from .ts" issue I'm encountering, but if i can resolve that issue then --outFile might let us allow us to do files one by one

[scheibo]

Ugh. TBH, at this point after wrestling with trying to get typed-JS and ts to play together I'm debating whether it might just be better to bite the bullet and create a feature branch where all 150 files are migrated to .ts in one go. "it shouldn't be too hard" given everything is already typed, the main concern would be doing the migration during a period of time where master isn't being updated too frequently and conflicts could be kept to a minimum.

[Zarel]

This would only work if none of the other typed-JS code depends on things within sim/ (which they don't because they definitely want sim/dex.js). And even then I'm not sure it would work because I don't know about how clever Typescript is with ordering (will it know to compile and emit code from sim-ts/ to sim/ so that all the other JS requires work? I highly doubt it). I also think we'd need to convert sim/s dependencies (data/, lib/streams.js) for it to compile.

It's unclear to me why this would be the case. Most of our dependencies are currently globals, anyway (for hotpatch compatibility), so we have some of the affordances that allow us to intermix things in client.

[scheibo]

I'm trying to convert sim/. I don't think it will work, but YOLO.

Very interesting the types of things Typescript catches with .ts filles that checkJs seems to miss:

Unless I'm mistaken and {Pokemon} [pokemon] in the JSDoc should not be translated as pokemon?: Pokemon in Typescript, it seems like the compiler is also better at actually noticing things that could be undefined (which, if we know they can't be in this particular circumstance we can trivially fix with action.pokemon!.position etc). My point being, it seems like we get better type checking in .ts

[scheibo]

Did a rough conversion for all of sim/ except for battle.js - I'll tackle it in the morning and start actually trying to compile things to prove why I'll actually probably need to convert all typed-JS in one go (I'd of course love to be wrong).

dex-data.js is the worst with all the fields, and Typescript really hates the Object.assign pattern being used - had to use tons of // @ts-ignore for error TS2565: Property 'Foo' is used before being assigned.. I have an idea for a Typescript-friendlier pattern, but just converting over the existing code as-is is my main goal - I plan to // @ts-ignore most new potential bugs identified by tsc with the .ts files and then go back later and try to eliminate as many as possible.

[Zarel]

You may be interested in my TypeScript-friendlier pattern:

https://github.com/Zarel/Pokemon-Showdown-Client/blob/master/src/battle-dex-data.ts

Although unlike server, client explicitly discards unrecognized data.

[scheibo]

Yup, thats basically it. Just do the Object.assigns to merge the data and moreData etc and come up with an any object, then pull fields off of it. Pretty easy, but I'm trying not to make any logic changes whatsoever, just rote conversion to limit the scope.