Typescript conversion

[ephread]

Tracking the TypeScript conversion.

• CallStack.js
• Choice.js
• ChoicePoint.js
• Container.js
• ControlCommand.js
• Divert.js
• Glue.js
• InkList.ts
• JsonSerialisation.js
• ListDefinition.ts
• ListDefinitionsOrigin.ts
• NativeFunctionCall.js
• Object.js
• PRNG.ts
• Path.ts
• Pointer.js
• PushPop.ts
• SearchResult.js
• StopWatch.ts
• Story.js
• StoryException.ts
• StoryState.js
• StringBuilder.ts
• Tag.ts
• Value.js
• VariableAssignment.ts
• VariableReference.js
• VariablesState.js
• Void.js

The conversion is fairly straightforward, albeit time-consuming. I touch very little of the previous code, unless my hand is forced by the type system.

That said, I'm nonetheless converting var to let or const (depending on the context) and I'm also replacing .forEach by for of loops (they're closer to their C# counterparts, they also allow returning early and Babel will take care of transpiling them).

If anyone wants to contribute, pick a file, let me know about it and make a PR against ephread/inkjs/typescript-conversion (or if @y-lohse merge this PR, a PR against y-lohse/inkjs/typescript). Tests should keep passing after each commit.

If the file have dependencies which haven't been converted yet, it's good practice to define the type as any and indicate the future type in a comment next to it (e. g. here).

[ephread]

As of 51d2f94, I've also made a couple of important changes to please the type system (and us). I've added large blocks of comments to explain the reasons, in the code. @y-lohse, you'll certainly want to review this and tell me if it fits your vision for the project!

InkList is now a Map

It makes the code closer to its C# counterpart, but comes with a tradeoff, since Javascript doesn't allow the creation of custom value-types (InkListItem is supposed to be a struct). Comments are here and there.

NullException can be thrown

TypeScript now complains when we try to pass a null value to a method which doesn't handle it, hence we're now throwing NullException (much like in C#) and providing an exit path when the type system requires it, rather than failing silently. See here.

TryGet functions

I'm trying to normalize the behavior of TryGet functions, using the type system. There's not much to say about it, other than I'm using the { result, exists } object which was previously introduced. See here.

---

I call dibs on Container.js and Object.js!

[y-lohse]

Cool, thanks! I'll review this later on.

In the meantime, I'd like to do some, to get the hang of it. So I'll take Choice and ChoicePoint if that's ok!

[y-lohse]

A bunch of general comments after trying some conversion myself:

• I think we'll eventually be able to leverage private and protected properties, which is very exciting! I had to workaround a few situations in the original port that hopefully can be cleaned up.
• Minor thing, but I think we should have a lint task that's separate from the build task.
• Possibly more controversial: I think we should turn off the prefer-const rule. const is nice, but I strongly dislike the trend of using it as much as possible in javascript, especially because it's not actually really immutable. I'd rather const be reserved for things that are, conceptually, a constant. Some people have even stronger opinion on this than me :D

[joethephish]

I was looking into TypeScript the other day, I’m looking forward to seeing this go in!

Are you planning to have full strict mode turned on? The non-nullables in particular have me quite excited :)

[ephread]

@y-lohse

• I think we'll eventually be able to leverage private and protected properties, which is very exciting! I had to workaround a few situations in the original port that hopefully can be cleaned up.

Jinx, that was my intent. At the moment I'm mostly making everything public, but once the conversion is complete, I plan to make a pass on all classes and set the proper access control modifiers.

• Minor thing, but I think we should have a lint task that's separate from the build task.

Do you want to remove the lint step from the build entirely, or do you mean that we should have an additional script handling the lint? I configured my editor so it's linting as-I-type, hence I never bothered creating a new npm script, but that's probably an oversight.

• Possibly more controversial: I think we should turn off the prefer-const rule. const is nice, but I strongly dislike the trend of using it as much as possible in javascript, especially because it's not actually really immutable. I'd rather const be reserved for things that are, conceptually, a constant. Some people have even stronger opinion on this than me :D

I just love that bit:

HiHaveYouTriedTypeScriptYouShouldReallyTryTypeScriptIWriteTypeScriptAndImFuckingObnoxiousImGonnaStickMyHeadInAMicrowaveAndScreamTypeScript™

I guess Have you tried TypeScript is the new You should use React (Native) (probably even coming from the same folks on Twitter) 😂. I plead guilty on this one!

I hold zero opinion on the matter, if you feel that we should keep const for constant value-types, I'm on board. We should probably use #161 to discuss styling (in fact, I'm going to comment there, NOW!).

---

@joethephish

If you mean --strict, it's already turned on 😉. That's why we're forced to throw exceptions (for now) in a couple of places.

[joethephish]

Oh awesome! Very cool.

[y-lohse]

I had completely forgotten about the typescript rant in the middle until after I posted the link haha 😁

Do you want to remove the lint step from the build entirely, or do you mean that we should have an additional script handling the lint?

I mean having an additional, separate step for linting. We can run it manually whenever we want to, and it would be run automatically by travis (and would block builds / releases). It makes sense to have both during the initial port to typescript, but when you're bug-hunting or quickly testing stuff, it's annoying as hell to get blocked by the linter when you just want a quick test run :)

[ephread]

We could actually remove it altogether from the build script even during the conversion phase. It was perhaps a bit overzealous of me to add it in the first place, as I currently want to test something out and find myself prevented from doing so by the linter.

I get it now 😀.

[ephread]

As of 1ea543c, I've kept turning plain old regular Javascript object into Map where it made sense. Since I need to update some portion of the Javascript files as well, to keep the test running, it makes the process a bit more nerve-wrecking. But it's manageable.

I didn't anticipate that I would need to do this in the first place, but it's definitely easier/better than defining interfaces for all the dictionaries or using any (TypeScript doesn't like when objects don't define their available keys through interfaces).

Type cast / Type assertion

I've also introduced the TypeAssertion.ts file, which provides a couple of methods for casting objects at runtime while remaining friendly with the type system.

As said on #177, asOrNull (might not be aptly named, but we can change it later) matches the behavior of as in C# and asOrThrow matches the behavior of (type).

asINamedContentOrNull matches the behavior of as when casting to INamedContent, it's duck typing for the win and a reuse of the previous work laid out by @y-lohse (thanks, by the way, it would probably have taken a while for me to consider that option).

I've organized these helpers as plain exported functions, but feel free to suggest another architecture.

Debug.Assert

I've also organized the console.warn into the Debug.ts file, which proved to be helpful during the conversion (when making sure that non typed files used Map instead of object). Bonus point, it makes the code looks more like its C# counterpart (especially around this bit).

Again, I didn't plan to implement all these things during the conversion. I made the mistake of not anticipating that that they would be required 😓.

---

The migration diff is really hairy, as it combines simple type annotation with object-to-map / runtime casting changes. The diff will definitely be unmanageable as-is if @y-lohse wants to review line-by-line. Let me know if you want me to create another PR once it's completed and rebase all the changes in a one-file-per-commit fashion.

[y-lohse]

I don't think the line by line review will be necessary (or doable :D), but the new security that typescript brings + the test suite gives me enough confidence that we won't ship anything broken.

So it's only the "extras" like conversions to Maps, Debug. Assert etc. If you can keep highlighting them here as you go that will be enough for the review at the end. But I should note that I'm totally happy with the changes to far.

---

I'd like to merge this PR, and copy over the progress tracker and various informations to a new PR that would merge y-lohse/inkjs/typescript into master. Would that make your work any harder?
It's purely because we had some discussions over on my PR on your fork, and I think it'd be better of all the discussion happens on the same repo. So if it causes you any inconvenience, I'm more than happy to keep working like this!

---

I'd like to do CallStack next, since I've already touched it while porting the Choicepart :-)
edit: nevermind it's huge and has lots of dependances. I'll go with them first, so let's say Pointer and Value.

[ephread]

I'd like to merge this PR, and copy over the progress tracker and various informations to a new PR that would merge y-lohse/inkjs/typescript into master. Would that make your work any harder?
It's purely because we had some discussions over on my PR on your fork, and I think it'd be better of all the discussion happens on the same repo. So if it causes you any inconvenience, I'm more than happy to keep working like this!

I agree wholeheartedly, the current setup is a bit clumsy. I should probably have created a PR with the bare minimum and wait for you to merge it before iterating.

I've merged your changes, so you can merge typescript-conversion whenever you're ready.

---

I'd like to do CallStack next, since I've already touched it while porting the Choicepart :-)
edit: nevermind it's huge and has lots of dependances. I'll go with them first, so let's say Pointer and Value.

Haha, yeah, Callstack, Story, StoryState and (the worst offender) JsonSerialisation will definitely be among the last ones to be converted.

Go ahead with these, I'll do all the little ones remaining in the meantime (Void, ControlCommand, Glue, VariableReference)

Edit:
That went much faster than expected. I'll do Divert once you'll have converted Pointer.

[y-lohse]

To be continued in #183 !