Typescript Migration Tasks Management

[wassertim]

Hey Typescript Team,

I created an issue for us to manage the progress of typescript migration tasks. Would be nice if you could read it and discuss any suggestions and improvements here.

The issue is here: #884

Would be nice if you don't comment there if it is not related to changing of status of a task. Let's discuss here in this thread

FYI: @rvilarl @0xfe @mscuthbert

[wassertim]

@0xfe You suggested Trello for this but I don't think it will serve us well, since it is third party. So we would have to switch to trello and back (inconvenient). I took your's other suggestion with check boxes which does show a nice progress.

[wassertim]

This is how the progress bar looks like:

[wassertim]

I understand, that there is a temptation to add some refactorings and improvements to the migration. But this makes a review process so much harder. @rvilarl would be nice if we could avoid it and do only one thing at a time. That was the main reason in the first place, remember?

Just create as many refactoring topics as you want. But please let's don't mix them. Thanks

[rvilarl]

@wassertim I am only discussing typescript related topics, I am colleting the other ones in #865

@wassertim @0xfe @mscuthbert perhaps we should agree what is a typescript topic :)

1) Is any avoidance a typescript related topic? I think YES
https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html
We are in a migration and we will be forced to leave some instances of any, but they should be minimal, just where we find no other option.
"typescript + any = javacript" this is not our goal

@sschmidTU you have experience with TS, what do you think?

[wassertim]

Good, an Issue is a good place for that. PR is not for discussion but for a code review and should stay clean.

[0xfe]

My preference here: Yes, any avoidance is TS related, and okay to discuss in reviews. Getting the types right is prob one of the most important parts of the migration.

Note that, the default stance of avoiding any is sound, however there are places where we can tradeoff convenience and make exceptions. (And we should strive to keep it minimal.)

[0xfe]

@wassertim I agree that refactoring should be separate PRs, however I don't typically categorize typing additions as refactoring (unless it changes a significant amount of code.)

[sschmidTU]

Yes, i agree that you should try to avoid any. The connection to that in the PR 882 just wasn't obvious before you clarified it, rvilarl. I also think that in some places it's hard to avoid, as 0xfe said.

[ronyeh]

To help with migration from JS => TS, I sometimes insert a line into the beginning of a function like so:

  expect(rules) {
    console.log(`expect(rules: ${typeof rules})`);

function flattenMatches(results) {
  console.log(`flattenMatches(results: ${typeof results})`);

  parse(line) {
    console.log(`parse(line: ${typeof line})`);

Then I refresh the vexflow tests page (and also my own test pages), and get console output like:

expect(rules: function)
expectOne(rule: object, maybe: boolean = false)
expect(rules: function)
expectOne(rule: object, maybe: boolean = false)
expect(rules: function)
matchToken(token: string, noSpace: boolean = false)
expect(rules: function)
matchToken(token: string, noSpace: boolean = false)
flattenMatches(results: object)
............... LONG FILE ....................
expect(rules: function)
matchToken(token: string, noSpace: boolean = false)
expect(rules: function)
expectZeroOrMore(rule: object)
expectOneOrMore(rule: object, maybe: boolean = false)
expectOne(rule: object, maybe: boolean = false)
expect(rules: function)
matchToken(token: string, noSpace: boolean = false)
matchFail(returnPos: number)
expect(rules: function)
expectOne(rule: object, maybe: boolean = false)

I sort and unique the lines in my favorite text editor, and get:

expect(rules: function)
expectOne(rule: object, maybe: boolean = false)
expectOneOrMore(rule: object, maybe: boolean = false)
expectZeroOrMore(rule: object)
flattenMatches(results: object)
matchFail(returnPos: number)
matchToken(token: string, noSpace: boolean = false)
parse(line: string)

This helps me figure out which types are expected. (Of course, the TypeScript compiler and VS code also help greatly if/when I use the wrong type!)

[mscuthbert]

This is brilliant. I'm going to do this on all future projects. Maybe also adding a check that if it's an object to give me the keys.

[ronyeh]

Yup! Depending on what I'm trying to inspect, I've tried some of the following on the incoming parameters:

JSON.stringify(...)
Array.isArray(...)
Object.keys(...)
console.dir(...)

I have also considered using a Set object so I don't have to console.log() and manually sort / unique the output. I would just put the output as strings into a Set, and then assign that Set to window["hello"]. After I run the test page, I can inspect window.hello to see all the different call signatures.

One other fun hack when converting things to TypeScript is you can set a method to private, or just change the name of the method. (Or you could even temporarily rename the class.) Then you can rely on the TS compiler's errors to tell you which test cases or which external classes call that method or use that class.