Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TS 2.4 PromiseLike Errors With Async in VSCode Codebase #30216

Closed
mjbvz opened this issue Jul 6, 2017 · 5 comments · Fixed by #30228
Closed

Fix TS 2.4 PromiseLike Errors With Async in VSCode Codebase #30216

mjbvz opened this issue Jul 6, 2017 · 5 comments · Fixed by #30228
Assignees
Labels
engineering VS Code - Build / issue tracking / etc.
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Jul 6, 2017

Building VSCode with TS 2.4, we're seeing about 10 errors in async functions around TPromise:

[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/services/textmodelResolver/test/textModelResolverService.test.ts(136,47): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/services/textmodelResolver/test/textModelResolverService.test.ts(136,47): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
  Type 'TPromise<T | PromiseLike<T>>' is not assignable to type 'PromiseLike<T>'.
    Types of property 'then' are incompatible.
      Type '{ <U>(success?: (value: T | PromiseLike<T>) => TPromise<U>, error?: (err: any) => TPromise<U>, pr...' is not assignable to type '<TResult1 = T, TResult2 = never>(onfulfilled?: (value: T) => TResult1 | PromiseLike<TResult1>, on...'.
        Types of parameters 'success' and 'onfulfilled' are incompatible.
          Types of parameters 'value' and 'value' are incompatible.
            Type 'T | PromiseLike<T>' is not assignable to type 'T'.
              Type 'PromiseLike<T>' is not assignable to type 'T'.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts(119,29): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts(153,38): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts(429,29): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts(447,29): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/parts/extensions/electron-browser/extensionsViewlet.ts(303,28): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/platform/extensionManagement/node/extensionGalleryService.ts(358,44): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.
[16:17:43] Error: /Users/matb/projects/vscode/src/vs/workbench/electron-browser/extensionHost.ts(355,118): Type 'typeof TPromise' is not a valid async function return type in ES5/ES3 because it does not refer to a Promise-compatible constructor value.

Steps to reproduce

  1. Using VScode insiders.
  2. Clone vscode code base
  3. Run ./scripts/npm install
  4. Open vscode codebase in vscode
  5. Open effected file
  6. Ensure that TS 2.4.1 is active in your workspace by looking in the lower right status bar for the TS version number
@mjbvz mjbvz added the engineering VS Code - Build / issue tracking / etc. label Jul 6, 2017
@mjbvz mjbvz added this to the July 2017 milestone Jul 6, 2017
@mjbvz mjbvz self-assigned this Jul 6, 2017
@mjbvz
Copy link
Collaborator Author

mjbvz commented Jul 6, 2017

@rbuckton or @andy-ms Can someone on the TS team please take a quick look at these errors? I've investigated but the error don't make much sense to me. I don't know where TS is getting the type T | PromiseLike<T> from in textModelResolverService.test.ts for example

This only started showing up in TS 2.4. My current thought is that we need to stop using async for TPromise methods

@mjbvz mjbvz changed the title TS 2.4 PromiseLike Errors With Async Fix TS 2.4 PromiseLike Errors With Async in VSCode Codebase Jul 6, 2017
mjbvz added a commit to mjbvz/vscode that referenced this issue Jul 6, 2017
Fixes microsoft#30216

**Bug**
While compiling vscode with TS 2.4, there were around 10 errors reported about async functions that return a `TPromise`.

**Fix**
I'm checking with the TS team to see if these errors are expected or not. Until then, I believe we need to stop using `TPromise` with async functions. This change changes all instances that use `async` to instead use `then`.
@ghost
Copy link

ghost commented Jul 7, 2017

Hi, it looks like you can get rid of the error by changing TPromise's constructor definition to be compatible with Promise's, as in:
constructor(init: (complete: (value: V | PromiseLike<V>) => void, error: (err: any) => void, progress: ProgressCallback) => void, oncancel?: any);

That said, the error was complaining about then and not constructor, so it took me a while to figure this out! I'll report a bug on TypeScript.

The reason you are just getting this error now in TS2.4 is due to microsoft/TypeScript#15104, since we are now checking the types inside of callbacks covariantly (previously it worked since V | PromiseLike<V> is a supertype of V).

@rbuckton
Copy link
Member

rbuckton commented Jul 7, 2017

In 2.4.1 we are stricter about the types for callbacks. It looks like the issue is the definition of TValueCallback<T> in winjs.base.d.ts. Changing the following definition seems to address the error:

export interface TValueCallback<T> {
  (value: T | Thenable<T>): void;
}

In general, we've made a number of improvements in the overall type definitions for the ES6 Promise type that have apparently not made their way into the winjs declarations. While not necessary to fix the above error, in the future I would recommend the following changes:

  • Replace Thenable<T> (in vscode.d.ts) with a type alias to PromiseLike<T> (for backwards compatibility)
  • Replace all references to Thenable<T> to PromiseLike<T> in the vscode code base (the compiler has optimizations in async function code paths for type references to the built in PromiseLike<T> type).
  • Replace Promise, TPromise, and PPromise in winjs.base.d.ts with the following declaration that leverages type parameter defaults:
export declare class Promise<T = any, TProgress = any> {
  constructor(
    executor: (
      resolve: (value: T | PromiseLike<T>) => void, 
      reject: (reason: any) => void, 
      progress: (progress: TProgress) => void) => void,
    oncancel?: () => void);

  public then<TResult1 = T, TResult2 = never>(
    onfulfilled?: (value: T) => TResult1 | PromiseLike<TResult1>, 
    onrejected?: (reason: any) => TResult2 | PromiseLike<TResult2>,
    onprogress?: (progress: TProgress) => void): Promise<TResult1 | TResult2, TProgress>;

  public done(
    onfulfilled?: (value: T) => void,
    onrejected?: (reason: any) => void,
    onprogress?: (progress: TProgress) => void): void;

  public cancel(): void;

  public static as<T, TPromise extends PromiseLike<T>>(value: TPromise): TPromise;
  public static as<T>(value: T): Promise<T>;

  public static is(value: any): value is PromiseLike<any>;

  public static timeout(delay: number): Promise<void>;

  public static join<T>(promises: (T | PromiseLike<T>)[]): Promise<T[], { Key: string, Done: true }>;
  public static join<T>(promises: { [n: string]: T | PromiseLike<T> }): Promise<{ [n: string]: T }, { Key: string, Done: true }>;

  public static any<T>(promises: (T | PromiseLike<T>)[]): Promise<{ key: string; value: Promise<T>; }>;

  public static wrap<T>(value: T | PromiseLike<T>): Promise<T>;

  public static wrapError(error: Error): Promise<never>;

  /**
   * @internal
   */
  public static addEventListener(event: 'error', promiseErrorHandler: (e: IPromiseError) => void);
}

export { 
  Promise as TPromise, 
  Promise as PPromise
};

I can submit a PR against VSCode for this if you would like.

@mjbvz
Copy link
Collaborator Author

mjbvz commented Jul 7, 2017

Thanks. The suggested TValueCallback change does seem to fix things. I'll take it into vscode.

@alexandrudima @jrieken @bpasero (Not sure of correct owners here. Please add anyone else that may have input) Can you please take a look at a Ron's other suggested changes to see if these are something we want to investigate

mjbvz added a commit to mjbvz/vscode that referenced this issue Jul 7, 2017
Fixes microsoft#30216

Changes the TPromise callback interface as suggested by microsoft#30216 (comment) to fix async functions when compiling using ts 2.4+
@joaomoreno
Copy link
Member

This sounds good to me, but I'm sure @jrieken would like to give his $0.02 about that in the API.

mjbvz added a commit that referenced this issue Jul 7, 2017
Fixes #30216

Changes the TPromise callback interface as suggested by #30216 (comment) to fix async functions when compiling using ts 2.4+
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engineering VS Code - Build / issue tracking / etc.
Projects
None yet
3 participants