[Engineering] TypeScript 3.7 migration plan

[stevengum]

Versions

What TypeScript version are you using? 3.5.3

TypeScript 3.7 contains breaking changes when compared to 3.5. Our last two minor releases (4.5, 4.6) were shipped with Typescript 3.5.

TypeScript 3.7 has several new features that could help make the team's lives easier, especially for those coming from C# and writing in TypeScript.

If we use the .d.ts files from TypeScript 3.7, it contains a breaking change regarding the declaration of get/set accessors in an ambient context.

As such, we can explore using different packages to generate .d.ts files if we as a dev team want to move to TS 3.7, but keep our current types. One package suggested by @christopheranderson is @microsoft/api-extractor (GitHub Repo)

---

Additional context: #1436

[johnataylor]

There are 16 setters in the SDK. These could potentially cause breaks in application code if is has derived from these classes. As a general statement, it would be very odd code to derive and then set these properties in the
definition of the class. getters and setters have been introduced generally when the underlying property is not trivial and but requires some additional steps such as walking into an underlying object or setting multiple properties. As a general statement, these circumstances are often for properties that are set post instance creation, an example being 'responded' it would just be odd to set this at instance creation time.

The problems around derivation are explained here:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier

The recommendation is to update to the 3.7 compiler and also to set the "useDefineForClassFields": true in the tsconfig.

The alternative is to introduce additional steps into our tool chain whereby we generate the .d.ts files via another tool (or previous version of the compiler). This does not seem justified at this stage.

Here is an exhaustive list of the properties.

1. C:\private\v4\botbuilder-js\libraries\adaptive-expressions\src\constant.ts

l.27
There are no derived classes in the SDK.

FIX:
README / release notes.

Classes that derived from Constant should *not*
assign to to value property within the
definition of the class. Instead they should do so
in the constructor body. (We don't have an example of anyone deriving from Constant.)

C:\private\v4\botbuilder-js\libraries\botbuilder-core\src\botAdapter.ts

l.115

FIX:
README / release notes.

Classes that derived from BotAdapter should *not*
assign to to onTurnError property within the
definition of the class. Instead they should do so
in the constructor body. Or, as we do in the samples, assign to the property on teh instance of the adapter after you have created it.

C:\private\v4\botbuilder-js\libraries\botbuilder-core\src\turnContext.ts

l.719

responded is a propery that manages an underlying _respondedRef which private and only manipulated with the accessors.

FIX:
README / release notes.

Its not anticipated that code derives from TurnContext, however if it does
*and* decides to initialise the responded to true it should do so only with
an explicit call to the responded property from within the constructor body of the
derived class. Having said that we can't foresee any circumsatnces where anyone
would want to do this anyhow. 

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs\src\dialog.ts

l.329

FIX:
README / release notes.

Classes that derived from Dialog should *not*
assign to the id property within the
definition of the class.
Instead they could do so in the constructor body.
However the correct code (and the code we have in the SDK) is to pass the
id in to the base class as the argument to super()

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs\src\dialog.ts

l.344

FIX:
README / release notes.

Classes that derived from Dialog should *not*
assign to the telemetryClient property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.
However, it is far more likely that the property is assigned after the
dialog instance is created. In which case there is no problem.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs\src\dialogContainer.ts

FIX:
README / release notes.

Classes that derived from DialogContainer should *not*
assign to the telemetryClient property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.
However, it is far more likely that the property is assigned after the
dialog instance is created. In which case there is no problem.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs\src\dialogManager.ts

FIX:
README / release notes.

Classes that derived from DialogContainer should *not*
assign to the rootDialog property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs\src\dialogSet.ts

FIX:
README / release notes.

Classes that derived from DialogSet should *not*
assign to the telemetryClient property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.
However, it is far more likely that the property is assigned after the
dialog instance is created. In which case there is no problem.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs-adaptive\src\adaptiveDialog.ts

NOTE:
TODO: checck derived types in the SDK.

FIX:
README / release notes.

Classes that derived from AdaptiveDialog should *not*
assign to the schema property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs-adaptive\src\recognizers\intentPattern.ts

FIX:
README / release notes.

Classes that derived from IntentPattern should *not*
assign to the intent or pattern properties within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs-adaptive\src\recognizers\entityRecognizers\regexEntityRecognizer.ts

FIX:
README / release notes.

Classes that derived from RegexEntityRecognizer should *not*
assign to the pattern property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs-adaptive-testing\src\adaptiveTestAdapter.ts

FIX:
README / release notes.

Classes that derived from AdaptiveTestAdapter should *not*
assign to the enableTrace property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botbuilder-dialogs-declarative\src\resources\folderResourceProvider.ts

FIX:
README / release notes.

Classes that derived from FolderResourceProvider should *not*
assign to the emitter property within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

C:\private\v4\botbuilder-js\libraries\botframework-connector\src\auth\appCredentials.ts

FIX:
README / release notes.

Classes that derived from AppCredentials should *not*
assign to the oAuthEndpoint or oAuthEndpoint properties within the
definition of the class.
Instead they could do so in the constructor body of the derived class.

[johnataylor]

Just a quick comment on motivation - other than it's nice to be more current :-). We do a lot of porting between languages and the recent additions to TypeScript include optional chaining amongst other things. This would really help in closing the gap between the TypeScript and C# implementations.

The risk of breaking changes in people code seems very low. There are always inherent risks with changes, looking through all the occurences of the use of setters, it seems very unlikely that anything would break.

[johnataylor]

This a change that we should make at the very beginning of the R11 milestone (note the intention of this change is specifically to benefit the development team working on the SDK - there is customer benefit but it is secondary).

Additional detail:

• with this change there are failing unit tests in BotFrameworkAdapter - more details to follow
• also setting the tsconfig useDefineForClassFields to true breaks the build of adaptive-expressions because the SDK code overrides an antlr property with a getter in a number of places. Either we need to not apply this property to this module or refactor - this needs further investigation.

[johnataylor]

Moving to R11. But note this issue should be treated with some priority in the milestone and address at the beginning or not at all.

[joshgummersall]

I've researched https://github.com/sandersn/downlevel-dts as a suitable approach. It is also used by other MSFT open source projects.