How should the code review checklist change given Typescript?

[zepumph]

No description provided.

[zepumph]

This was discussed during today's dev meeting about typescript conventions.

@samreid: We have https://github.com/phetsims/phet-info/blob/7100adc9d8bf5b79d1aa9da38b1f8302f3b059c1/doc/typescript-conventions.md, which I think of as code review items, but they aren't linked to the document yet.

@zepumph: Perhaps just an item in the CRC to link to that document?

@samreid: then perhaps make the typescript conventions document into checkboxes so you can copy paste them into a side issue of a code review.

@samreid, I think we should modularize the code review checklist.

@zepumph, the coding conventions doc is 5/8 of the document already, can we split that out? UPDATE: see phetsims/phet-info#186, the coding conventions were split out to their own document.

[zepumph]

Ok, the CRC has a checkbox to go look at the typescript conventions doc now:

From here, I feel like another important piece is to look into what can be shed for the document because of TypeScript. If not removed, perhaps we want to move some content into a "JavaScript only" section, because it doesn't apply to new code being written in Javascript. Some items that come to mind:

In Coding Conventions:

• "type expressions" and "visibility annotations" sections are javascript-specific.
• This is outdated:

  For constructors, use parameters for things that don’t have a default. Use options for things that have a default value. This improves readability at the call site, especially when the number of parameters is large. It also eliminates order dependency that is required by using parameters.

• The checkbox starting in this quote mentions outdated extend/merge calls:

  When options are passed through one constructor to another, a "nested options" pattern should be used

• Self should be replaced with arrow functions:

  If references are needed to the enclosing object, such as for a closure, self should be defined

• Javascript-specific:

  Constructor and function documentation. Parameter types and names should be clearly specified for each constructor and function using @param annotations.

I didn't see any other changes to be done in the CRC checklist.

[jonathanolson]

JO and CM didn't see anything else that should be added to the checklist. @zepumph anything else that should be added in your opinion? Feel free to close if not.