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

Bloomberg feedback on TypeScript 4.5 Beta #46382

Closed
mkubilayk opened this issue Oct 15, 2021 · 2 comments
Closed

Bloomberg feedback on TypeScript 4.5 Beta #46382

mkubilayk opened this issue Oct 15, 2021 · 2 comments
Assignees
Labels
Discussion Issues which may not have code impact

Comments

@mkubilayk
Copy link
Contributor

This issue has been created by the team responsible for the infrastructure behind most of Bloomberg's TypeScript code.

We have tested TypeScript 4.5 Beta on our TypeScript codebase. The current results indicate, once again, a fairly easy upgrade.

  • We did not need any change in our integration code using TypeScript compiler APIs.
  • JavaScript output slightly changed (target: "esnext"). These are explained in detail below.
  • Declaration output slightly changed. These are explained in detail below.
  • Some of our TypeScript code required changes to make it compile with TypeScript 4.5 Beta. These are explained in detail below.
  • We are still far from being able to use importsNotUsedAsValues: "error", preserveValueImports and isolatedModules at the same time.

Impact: Emit

# Change Affects Release notes Packages affected Introduced in
1 JSX transform spreads children JS Emit Not announced <1% #45693
2 Promise.all overload for any DTS Emit Announced <1% #45350

1. JSX transform spreads children

This is the only change in JavaScript output in our codebase. It is not a breaking change in semantics as React.createElement can handle both cases.

// TS input
() => <>{...[]}</>;
// JS output in TS 4.5 Beta
() => React.createElement(React.Fragment, null, ...[]);

Playground in TS 4.5 Beta

// JS output in TS 4.4.3
() => React.createElement(React.Fragment, null, []);

Playground in TS 4.4.3

2. Promise.all overload for any

This is the only change in declaration output in our codebase. It is not explicitly mentioned in the release notes. It might be possible to handcraft some examples where this could be a breaking change but that is not the case for our codebase. This change looks harmless.

// TS input
function f() { return Promise.all([] as any); }
// DTS output in TS 4.5 Beta
declare function f(): Promise<{
    [x: string]: any;
}>;

Playground in TS 4.5 Beta

// DTS output in TS 4.4.3
declare function f(): Promise<[unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown]>;

Playground in TS 4.4.3

Impact: Type Checking

# Change Release notes Packages affected Introduced in
1 Awaited type & promise signature change Announced <1% #45350
2 Change of inference with noImplicitAny: false Not announced <1% #45350
3 Edge case with generics and indexing Not announced <1% #41821

The overall impact of this version is very small and we can proactively update all affected packages in a forward-compatible way. However, we do think that some corner cases are worth looking at as they break here.

1. Awaited type & promise signature change

This is a documented change that seems to be breaking in very small corner cases.

Here is a first example where Awaited trips up with control flow analysis in TS 4.5 Beta:

declare function deepEqual<T>(actual: T, expected: T): void;

export async function testRequestSuccess<T>(resultP: Promise<T>, expected: T) {
    let actual;
    try {
        actual = await resultP;
    } catch (_e) {}

    // The type parameter of `deepEqual` is inferred based on the first argument.
    // TypeScript 4.4 calculates `T | undefined`, so passing `expected: T` is okay.
    // TypeScript 4.5 calculates `Awaited<T> | undefined`, which can't have `T` assigned to it.
    // Removing the earlier try-catch gets things working because `actual`'s type is just `Awaited<T>`,
    // which you *can* assign `T` to.
    deepEqual(actual, expected);
}

Playground in TS 4.4.3 | Playground in TS 4.5 Beta

Additionally, the change in type signature of Promise has brought up another issue with an explicit Promise.all typing:

// This call breaks in TS 4.5 Beta because
// multiple type variables are no longer supported
Promise.all<string, number>(["a", 1]);

Playground in TS 4.4.3 | Playground in TS 4.5 Beta

Given the low prevalence of those use cases, we can easily convert them to the new form:

Promise.all<[string, number]>(["a", 1]);

Playground in TS 4.5 Beta

2. Change of inference with noImplicitAny: false

Some of our projects have the noImplicitAny: false flag set. In this situation, some unwanted inference seems to happen in TS 4.5 Beta:

interface RejectedValue {
    status: "rejected";
    reason: Error | any;
}
interface FulfilledValue {
    status: "fulfilled";
    value: unknown;
}
type SettledValue = RejectedValue | FulfilledValue;
type SettledPromises = Promise<SettledValue[]>;

// This example requires `"noImplicitAny": false`.
export default function allSettled(promises): SettledPromises {
    const wrappedPromises = promises.map((p) =>
        Promise.resolve(p)
            .then((value) => ({ status: "fulfilled", value }))
            .catch((reason) => ({ status: "rejected", reason }))
    );
    return Promise.all(wrappedPromises);
//  ^ TS2322
//    Type 'Promise<{ [x: string]: any; }>' is not assignable to type 'SettledPromises'.
}

Playground in TS 4.4.3 | Playground in TS 4.5 Beta

In this example, return value is inferred to have Promise<{ [x: string]: any; }> type. This conflicts with the type annotation.

3. Edge case with generics and indexing

We found an instance where behavior differs from the previous version. The following example is contrived but is extracted from a much larger use case in our codebase.

type TypesOf<T extends Object> = T[keyof T]
type MatchingKeys<T extends Record<string,any>,  Condition>
    = TypesOf<{ [K in keyof T ]: T[K] extends Condition ? K : never }>


export interface Model {
    foo: string;
    bar: Array<string>;
}

type Extracted<A extends Array<any>> = unknown;
export declare function extract<
    M extends Model,
    K extends MatchingKeys<M, Array<any>>
>(m: M): Extracted<M[K]>;
//                 ^ TS2344
//                   Type 'M[string]' is not assignable to type 'any[]'.

Playground in TS 4.4.3 | Playground in TS 4.5 Beta

We will attempt to simplify it in the larger case to avoid this situation. However, this is still a breaking change for this edge case.

Additional Remarks: preserveValueImports

We are attempting to enable a few compiler options around modules that simplify the TS -> JS transform.

In order to do so we attempted to set importsNotUsedAsValues: "error" and preserveValueImports. We already have isolatedModules fully enabled and importsNotUsedAsValues: "error" partially enabled.

40% of our packages are ready for those options as-is while the remaining 60% are at least getting one TS1444 error or more. This is to be expected as many users import a mix of values and types on a single line.

type Modifiers on Import Names will be helpful to resolve those errors and promote those options further.

A code fix that is capable of inserting type modifiers where appropriate would be very valuable here. It would turn the following code:

import { value, Type } from "./lib";
//              ^ TS1444
// 'Type' is a type and must be imported using a type-only import when 'preserveValueImports' and 'isolatedModules' are both enabled.

into

import { value, type Type } from "./lib";
// no errors 🎉

If any of these changes do not look intentional/right and should be considered as a bug, we can open issues.

Credits to @scottrobertwhittaker, @rricard, @tchetwin and @robpalme for helping with this write up.

@andrewbranch andrewbranch added the Discussion Issues which may not have code impact label Oct 15, 2021
@andrewbranch
Copy link
Member

Thanks as always for the detailed feedback! I can chime in on a couple things:

I think “Promise.all overload for any” and “Change of inference with noImplicitAny: false” are in fact the same issue, which is being tracked at #46169.

A code fix that is capable of inserting type modifiers where appropriate would be very valuable here.

I intended to add this codefix when I implemented type-only import specifiers, but must have forgotten. I think we can probably get this in before the RC.

@mkubilayk
Copy link
Contributor Author

We completed the upgrade soon after the stable release. We haven't seen any other major issues.

See our feedback on 4.6 Beta at #47814.

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

3 participants