Update Promise definition

[rbuckton]

As a follow up to #30216 (comment), this updates the definition of Promise in winjs.base.d.ts to use type parameter defaults and addresses errors in several files where either TypeScript could not pick the correct type or where the code using the promise was trying to force the wrong type (e.g. trying to explicitly force a TPromise<Something> into a TPromise<void>).

[msftclas]

@rbuckton,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

[rbuckton]

// CC: @mjbvz

[jrieken]

@microsoft/vscode everyone should take a quick look at this

[chrmarti]

I see a few additional <any>s, looks good otherwise.

[rbuckton]

Most of the changes outside of winjs.base.d.ts and monaco.d.ts were adding explicit type annotations in a few places where TypeScript could not determine the correct type.

There are only 3 places where I changed runtime behavior:

• https://github.com/Microsoft/vscode/pull/30268/files#diff-b91a2dd0f85ddbdca2c739202a1d558fR303
• https://github.com/Microsoft/vscode/pull/30268/files#diff-e47216dc5271da0e55e758944c7ece54R93
• https://github.com/Microsoft/vscode/pull/30268/files#diff-86fa163ef57a4ed5ef30dd44c69074f9R538

All three cases were added to correctly set the resolved value based on the expected return type of the containing method. In all three instances a TPromise<void> was expected; however, in each case the resulting Promise could resolve to an actual value, so the result would not have actually been a TPromise<void>. In each case, I added a .then(() => { /* drop resolved value */ }) which would explicitly drop the resolved value of the antecedent and ensure a correct void return type.

Alternatively, I can change the return type of the three methods above to something like a TPromise<void | X> or TPromise<any> to avoid any changes to actual runtime emit, but with possibly reduced type safety. I felt the additional continuation more correctly illustrated the intent.

[rbuckton]

Also note, I just submitted microsoft/TypeScript#17077, which I hope to discuss at the next TypeScript design meeting. If we decide to accept that change for TypeScript 2.5, we could further improve the Promise implementation in VSCode as TypeScript would do a better job inferring the correct return types in some of your more complex promise chains.

[dbaeumer]

Wasn't aware that you can have default values for generic parameters. Cool.

[jrieken]

Assigning @sandy081 - the runtime changes as listed here affect your code if I am not mistaken. From my side this looks good

[rbuckton]

@dbaeumer, it's something we added in 2.3: https://blogs.msdn.microsoft.com/typescript/2017/04/27/announcing-typescript-2-3/#default-type-arguments

[sandy081]

@rbuckton It makes sense to me dropping the values in following

• https://github.com/Microsoft/vscode/pull/30268/files#diff-b91a2dd0f85ddbdca2c739202a1d558fR303
• https://github.com/Microsoft/vscode/pull/30268/files#diff-86fa163ef57a4ed5ef30dd44c69074f9R538

You can also resolve to null in these cases.then(() => null). I think this is what we do when we resolve a promise returningvoid.

I would ask @chrmarti to confirm the change (assigning @chrmarti)

• https://github.com/Microsoft/vscode/pull/30268/files#diff-e47216dc5271da0e55e758944c7ece54R93

[rbuckton]

@sandy081 we treat null, undefined, and void differently in the compiler, though that isn't always evident if you are not using --strictNullChecks. I'm generally wary of using null due to the fact that typeof null === "object". Also, () => {} is fewer characters than () => {}, though I added the comment to explain the rationale.

[sandy081]

@rbuckton Good to know.

[mjbvz]

@rbuckton Sorry, this fell off my radar. Can you resolve the merge conflicts so we can merge this in. I can also take the PR if you'd prefer

[mjbvz]

Looks like the latest iteration has a few build errors: https://travis-ci.org/Microsoft/vscode/jobs/265409458

[rbuckton]

Yeah, I just noticed that. It is building cleanly locally. I'll look into it.

[rbuckton]

I'm trying to make sure I have the correct dependencies in node_modules and have been running into issues. I deleted node_modules and ran scripts\npm.bat install and I'm getting the following errors when installing oniguruma@6.1.1 as a dependency:

Creating library C:\dev\vscode\node_modules\oniguruma\build\Release\onig_scanner.lib and object C:\dev\vscode\node_modules\oniguruma\build\Release\onig_scanner.exp
onig-result.obj : error LNK2001: unresolved external symbol onig_region_free [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol onig_search [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol OnigEncodingUTF8 [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol onig_free [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol OnigDefaultSyntax [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol onig_error_code_to_str [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol onig_region_new [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
onig-reg-exp.obj : error LNK2001: unresolved external symbol onig_new [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
C:\dev\vscode\node_modules\oniguruma\build\Release\onig_scanner.node : fatal error LNK1120: 8 unresolved externals [C:\dev\vscode\node_modules\oniguruma\build\onig_scanner.vcxproj]
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\MSBuild.exe` failed with exit code: 1

I'm using node v8.1.4, npm 5.3.0, and Visual Studio 2015

[roblourens]

You might also want to delete stuff in .build and elsewhere, I sometimes have to git clean -xfd when updating dependencies. Also we are mostly still on npm 4.x so you might want to try that too.