Switching to typescript

[y-lohse]

Continuing #175!

• 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

Stuff to review (from #175 (comment) & #175 (comment))

Checks

• Check that all Object have been converted to Map where appropriate (and that it works properly, especially in InkList)
• Check Null Exception handling
• Check that const are only used for constants.
• Check all type assignments in conditionals.
• Check that all any make sense (and are not placeholders for other types).
• Check access modifiers.
• Check handling of ES6 proxies in VariablesState.
• Check all TODOs (#194 (comment)).

Cleanup & style

• Turn C# params into TypeScript rest parameters.
• Remove all original C# comments, keeping only those pertaining to the JavaScript code.
• Reorder members according to the original order, in all files.
• Convert C#'s as into asOrNull and (type) into hasOrThrow where applicable.
• Convert forEach loops into for...of loops.

Decisions

• Raising exception vs. having undefined behavior (related to JsonSerialisation and Value, e. g. (int) variable vs parseInt(variable)). -> Throwing in case of failed cast #227
• Explicitly annotating the return type vs. letting TypeScript infer it. -> inferring
• Use of TryGet functions. -> TryGet functions #222
• Use of throwNullException function.
• Porting all Debug.Assert from C#. -> Debug.Assert calls and debug messages #221
• Define the rules about where initialize properties. -> following the C# order as much as possible
• Decide what we do about the unified-signature rule

Changes from master since the port has started

297f0b2...master

• fb20bcf
• b13f723
• 50811a8
• 333854e
• b111404
• 1b79e48
• 00d9123
• 91b038a

[ephread]

@y-lohse Void, ControlCommand, Glue, VariableReference have been done in
0abef65 by the way!

[ephread]

So, Value is the next bottleneck as it needs to be converted before tackling NativeFunctionCall and VariableStates.

@y-lohse would you be able to add something like Member order and property init style to Stuff to review (as per #161, and ephread#1)?

[y-lohse]

Ah yes, thanks for the head's up. I'l do the Value conversion today!

[ephread]

No worries, wasn't trying to pressure you, though. It was just in case someone would jump in to help!

[ephread]

Now that Value is completed, who wants to do what? Let's synchronize @NQNStudios, @y-lohse!

[NQNStudios]

VariablesState and StoryState look the most interesting to me, in terms of better understanding the flow of Ink. StoryState depends on CallStack, though, so maybe it makes sense for me to start with VariablesState while someone else does CallStack.

[y-lohse]

Sounds good to me! @ephread what do you feel like tackling next? I might take NativeFunctionCall, but I don't really mind either way.

[ephread]

@y-lohse I don't mind either way, I guess I'm going to do Callstack then!

[ephread]

@y-lohse yet another bunch of items to add to Stuff to review.

Checks

• Check that const are only used for constants.
• Check all type assignments in conditionals.
• Check that all any make sense (and are not placeholders for other types).
• Check access modifiers.
• Check handling of ES6 proxies in VariablesState.

Cleanup & style

• Turn C# params into TypeScript rest parameters.
• Remove all original C# comments, keeping only those pertaining to the JavaScript code.
• Reorder members according to the original order, in all files.

Decisions

• Raising exception vs. having undefined behavior (related to JsonSerialisation and Value, e. g. (int) variable vs parseInt(variable)).
• Explicitly annotating the return type vs. letting TypeScript infer it.

[y-lohse]

Since we're almost done with the actual porting, I'll kick off discussions about the decisions. Let's try it all in this PR, we'll see if we need to split up discussions.

1. Raising exception vs. having undefined behavior (related to JsonSerialisation and Value, e. g. (int) variable vs parseInt(variable)).

I'm not sure what this one is about, can we have a bit more details?

2. Explicitly annotating the return type vs. letting TypeScript infer it.

Is there any upside to define the return types explicitly when TS can infer it? It looks like unnecessary extra work to me.

3. Use of TryGet functions.

I have to look into that one before I have an opinion myself, but if you have one, shoot!

4. Use of throwNullException function.

Nothing to complain about here — it highlights places were variables could be null, and makes the errors more explicit should they happen. The impact on readability is very manageable.

5. Porting all Debug.Assert from C#.

Again, i have to look into that one. But it looks like something that can be done afterwards? It also ties in with the use of the console in general.

6. Define the rules about where to initialize properties.

As in, whether we define them inside vs outside the constructor?

If so, I'm very much in favor of declaring them outside, specifically having the declaration order match the C# order.

7. Decide what we do about the unified-signature rule

So this is the base rule:

Warns for any two overloads that could be unified into one by using a union or an optional/rest parameter.

I suspect that forcing the unification will actually make declarations less readable. Am I the only one? Whereas I really don't mind the multiple declaration.

[ephread]

1. Raising exception vs. having undefined behavior (related to JsonSerialisation and Value, e. g. (int) variable vs parseInt(variable)).

I'm not sure what this one is about, can we have a bit more details?

For instance, (int) value will raise an exception while parseInt() will always return a value, albeit an hypothetically incorrect one. (see here (JS) and here (C#))

2. Explicitly annotating the return type vs. letting TypeScript infer it.

Is there any upside to define the return types explicitly when TS can infer it? It looks like unnecessary extra work to me.

I agree!

3. Use of TryGet functions.

I have to look into that one before I have an opinion myself, but if you have one, shoot!

Opening another issue on this topic may be a good idea!

4. Use of throwNullException function.

Nothing to complain about here — it highlights places were variables could be null, and makes the errors more explicit should they happen. The impact on readability is very manageable.

I'd like to make one change: removing the string parameter from throwNullException. Ideally, we'll update the test and get proper source maps, which will make tracking issue easier.

5. Porting all Debug.Assert from C#.

Again, i have to look into that one. But it looks like something that can be done afterwards? It also ties in with the use of the console in general.

Definitely, we could open another issue to deal with this. I kind of like the idea of being explicit, but maybe we should disable any console use in production (using a flag?).

6. Define the rules about where to initialize properties.

As in, whether we define them inside vs outside the constructor?

If so, I'm very much in favor of declaring them outside, specifically having the declaration order match the C# order.

That's correct. I think we should follow strictly what the C# does, and in the uninitialized properties, initialize them outside of the constructor.

[y-lohse]

Created #221 and #222 to track decisions that can be postponed.

[y-lohse]

@NQNStudios I think you ere the one commenting on proxies here? If so, do you remember what you had in mind?

[ephread]

"Truly let" 😂, you dumb computer.

[y-lohse]

Woops, good catch^^

[y-lohse]

We're getting there! Most of the remaining checklist are non-urgent things.

Just a few more questions for you @ephread :

1. There are some TODO markers in CallStack that look like dev hints. Is there actually something to do on them?
2. You mentioned wanting to remove the parameter from throwNullException. Do you want to do this as part of this PR or at some later point?
3. What is this one about?

[ephread]

We're getting there! Most of the remaining checklist are non-urgent things.

Awesome!

There are some TODO markers in CallStack that look like dev hints. Is there actually something to do on them?

It's a reminder to decide whether we should throw if the value is not of the correct type (to match the C# code). What do you think?

You mentioned wanting to remove the parameter from throwNullException. Do you want to do this as part of this PR or at some later point?

It can wait, as we need to refactor the tests first (to get the proper line report when the test fail). At the moment, the test suit run against the compiled & concatenated JavaScript file and doesn't pick up the source maps, resulting in funky line numbers.

What is this one about?

I believe it's a reminder to make sure we that check against the appropriate value. undefined might make more sense here, since the absence of value will be undefined, rather than C#'s null.

[y-lohse]

It's a reminder to decide whether we should throw if the value is not of the correct type (to match the C# code). What do you think?

Ah, ok! I opened an issue for this, but I think we should throw. Since there's an issue for that I'll consider it non blocking for the merge.

It can wait, as we need to refactor the tests first (to get the proper line report when the test fail). At the moment, the test suit run against the compiled & concatenated JavaScript file and doesn't pick up the source maps, resulting in funky line numbers.

Alright!

I believe it's a reminder to make sure we that check against the appropriate value. undefined might make more sense here, since the absence of value will be undefined, rather than C#'s null.

Ah yes, good catch!

[y-lohse]

Alright, it's time to merge this PR!

First of all I want to thank @ephread for leading this effort and doing the bulk of the work. Big thanks as well to @NQNStudios for participating in this effort.

The remaining items on the checklist are not blocking and can be sorted over time. Some already have dedicated issues, other will get them as we go. Keeping this PR open until everything is checked would drag things out forever.

Going forward, I'll focus on making sure the changes from ink v0.8.2 get integrated before releasing a new stable version, and then avoiding having to do huge changes in one go as much as possible.