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

Google feedback on TS 4.4-rc #45571

Closed
h-joo opened this issue Aug 25, 2021 · 3 comments
Closed

Google feedback on TS 4.4-rc #45571

h-joo opened this issue Aug 25, 2021 · 3 comments
Assignees
Labels
Discussion Issues which may not have code impact

Comments

@h-joo
Copy link
Contributor

h-joo commented Aug 25, 2021

This GitHub issue contains feedback on the TS 4.4-rc type changes from the team
that is responsible for keeping Google's internal software working with the
latest version of TypeScript.

After the iteration of reporting for 4.4-beta, we saw lots of failures removed in
4.4-rc compared to 4.4-beta, or found a way to migrate removed types properly
via DefinitelyTyped.

Executive summary

  • We do not expect to have significant difficulty in upgrading Google to TS
    4.4-rc.
  • Some changes to our TypeScript code are required to make it compile with TS
    4.4-rc.
    • Majority of the failure are related to lib.d.ts changes. We will either
      cast away the errors or introduce types from the DefinitelyTyped. If not
      present in the DefinitelyTyped, we also plan to add them.
    • The remaining new TS errors are few. Some seems to be legitimate source
      code errors but for others it might need inspection from the TS team
      (See Breaking changes which were not announced).
    • Detail sections below explain the changes to our code we expect to make
      to unblock the upgrade.

Impact summary (lib.d.ts Changes)

Change description Libraries affected
Element start to extend ARIAMixin 0.11%
OffscreenCanvas removed 0.015%
Navigator API changes 0.012%
CustomElementRegistry.get type change 0.012%
window.location.reload type change 0.012%
SpeechRecognitionEvent removed 0.009%

All changes in lib.d.ts were clearly announced. The above list only contains
changes which had big number of failures.

For the missing types, we will either migrate via DefinitelyTyped if that's
available, or trying to add them to the DT ourselves for the types that we still
think it's needed. Otherwise if it's a use of deprecated API we'll try to cast
away the errors for now.

Impact summary (others)

Change description Announced Libraries affected
Disallowing Uninitilized Member for null/void/never types no 0.009%
Broader Always-Truthy Promise Checks yes 0.006%
Abstract Properties Do Not Allow Initializers yes 0.003%
Infinite Type Instantiation no 0.002%

The Announced column indicates whether we were able to connect the observed
change with a section in the TS4.4-rc announcement.

Breaking changes which were not announced

Disallowing uninitialized member for null/void/never types

class Error {
  // Property 'a' has no initializer and is not definitely assigned in the constructor.(2564)
  a: void;
  // Property 'b' has no initializer and is not definitely assigned in the constructor.(2564)
  b: null;
  // Property 'c' has no initializer and is not definitely assigned in the constructor.(2564)
  c: never;
  // OK.
  d: undefined;
}

Above code used to work in TS4.3, but now triggers an error from 4.4-beta on.
Raising an error here seems to make a bit of sense, but maybe the error
message could be different if this is the intended behavior.

Infinite Type Instantiation

declare var obj: {key: IDBValidKey;};

declare namespace jasmine {
  interface Matchers<T> {
    toEqual(expected: Expected<T>): void;
  }

  type Expected<T> =|T|{
    [K in keyof T]: ExpectedRecursive<T[K]>;
  }

  type ExpectedRecursive<T> =|T|{
    [K in keyof T]: ExpectedRecursive<T[K]>;
  };
}

declare function expect<T>(actual: T): jasmine.Matchers<T>;

expect(obj).toEqual({key: 'value1'});

This seems to have been caused by the newly introduced recursive definition of
the IDBValidKey, and this causes an infinite type instantiation. There is a
reproduce in the TS Playground.

We also noticed that there might be a bug in playground as well, when you click
the above link from Chrome, it does not show a red line indicating an error,
but it does show an error from Firefox. In Chrome, it emits :
Uncaught Error: Maximum call stack size exceeded in the console instead.

@DanielRosenwasser
Copy link
Member

class Yadda {
  // Property 'a' has no initializer and is not definitely assigned in the constructor.(2564)
  a: void;
  // Property 'b' has no initializer and is not definitely assigned in the constructor.(2564)
  b: null;
  // Property 'c' has no initializer and is not definitely assigned in the constructor.(2564)
  c: never;
  // OK.
  d: undefined;
}

@h-joo I'm not seeing a regression from 4.4 - this seems to have the same issues in 4.2 for example.

@h-joo
Copy link
Contributor Author

h-joo commented Aug 25, 2021

My apologies, here's a correct reproduce :

class Yadda {
  // Property 'a' has no initializer and is not definitely assigned in the constructor.(2564)
  a: void;
  // Property 'b' has no initializer and is not definitely assigned in the constructor.(2564)
  b: null;
  // Property 'c' has no initializer and is not definitely assigned in the constructor.(2564)
  c: never;
  // OK.
  d: undefined;

  constructor(readonly e: string) {}
}

Nightly Playground

4.4-beta Playground

4.3.5 Playground

@DanielRosenwasser
Copy link
Member

I see, so it's a difference of whether there's a constructor with a parameter property. That's definitely a bug fix, but yes, it'll break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

4 participants