Skip to content

TS 3.4: Error when passing dynamically imported generic type as a function argument #30712

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

Open
srmagura opened this issue Apr 2, 2019 · 17 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@srmagura
Copy link

srmagura commented Apr 2, 2019

TypeScript Version: 3.4.0-dev.201xxxxx

Search Terms: TS2322, dynamic import, dynamic import never, dynamic import generic

Code

 .\node_modules\.bin\tsc --jsx react --lib es2017 --strict index.tsx
// ===== page.tsx ======
import * as React from 'react'

export interface PageProps {
    title: string
}

export class Page extends React.Component<PageProps> {
    render() {
        return <div>{this.props.title}</div>
    }
}

// ===== index.tsx =====
import * as React from 'react'
import{ PageProps, Page } from './page'

export function myFunction<TProps>(loader: () => Promise<React.ComponentType<TProps>>) {

}

// No error
myFunction(() => Promise.resolve(Page))

// No error
const loader: () => Promise<React.ComponentType<PageProps>> = () => import('./page').then(m => m.Page)

// Error
myFunction(() => import('./page').then(m => m.Page))

Expected behavior:

No compile error. This was the behavior in TS 3.3 and earlier.

Actual behavior:

There is an error after upgrading to TS 3.4:

index.tsx:14:18 - error TS2322: Type 'Promise<typeof Page | ComponentClass<never, any> | FunctionComponen
t<never>>' is not assignable to type 'Promise<ComponentType<PageProps>>'.
  Type 'typeof Page | ComponentClass<never, any> | FunctionComponent<never>' is not assignable to type 'C
omponentType<PageProps>'.
    Type 'ComponentClass<never, any>' is not assignable to type 'ComponentType<PageProps>'.
      Type 'ComponentClass<never, any>' is not assignable to type 'ComponentClass<PageProps, any>'.
        Type 'PageProps' is not assignable to type 'never'.

14 myFunction(() => import('./page').then(m => m.Page))
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  index.tsx:4:44
    4 export function myFunction<TProps>(loader: () => Promise<React.ComponentType<TProps>>) {
                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    The expected type comes from the return type of this signature.

Repro here: https://github.com/srmagura/ts-import-repro

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Apr 17, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.5.0 milestone Apr 17, 2019
@styu
Copy link

styu commented Apr 30, 2019

Are there any suggested workarounds for the above in the meantime? I would like to use the incremental build features from 3.4, but hitting this issue with some dynamic imports

@sandersn
Copy link
Member

sandersn commented May 20, 2019

Inference for then tries to infer both TResult1 and TResult2 from the below signature.

interface Promise<T> {
    then<TResult1 = T, TResult2 = never>(onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null, onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null): Promise<TResult1 | TResult2>;
}

The type we want is Promise<typeof Page>. But TResult2 is not inferred from the second argument — it's not given — so return type inference looks to the contextual type and gets React.ComponentType from myFunction.

This is arguably correct since then doesn't say what to do in case of failure, so a weird type like ComponentClass<never> might be what results. But it's not desired.

To get the desired type we could break then into two overloads:

interface Promise<T> {
    then<TResult1 = T>(onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null): Promise<TResult1>;
    then<TResult1 = T, TResult2 = never>(onfulfilled: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null, onrejected: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null): Promise<TResult1 | TResult2>;
}

Or we could modify return type inference to only apply to type parameters that have inferences from some argument, not just the return type. I think this would break some existing uses but they might be good breaks.

@rbuckton @ahejlsberg can you take a look to see whether you would rather

  • change return type inference
    vs
  • add a Promise.then overload
    vs
  • do nothing and declaring this correct

@sandersn
Copy link
Member

@styu

Workarounds:

  1. avoid context by extracting import('./page').then(m => m.Page):

    const loader = () => import('./app').then(m => m.Page)
    myFunction(loader)

    This avoids the return type inference since there is no contextual type to infer from.

  2. provide a real or fake onrejected:

    myFunction(() => import('./app').then(m => m.Page, e => null as never))

    This one is fake — null as never disappears nicely in the type system — but a real app should probably provide a fallback page in case the import fails.

@sandersn
Copy link
Member

sandersn commented May 22, 2019

Here's a much smaller repro that doesn't depend on React or Promise:

declare class Component<P> { }
interface ComponentClass<P = {}>  {
    new (props: P, context?: any): Component<P>;
}

declare function myFunction<TProps>(loader: P<ComponentClass<TProps>>): void;

export class Page extends Component<{}> {}
interface P<T> {
    then<T1=T, T2=never>(pass?: ((value: T) => T1), fail?: ((reason: any) => T2)): P<T1 | T2>;
}

declare var pp: P<{ Page: typeof Page }>
myFunction(pp.then(m => m.Page))

const loader = pp.then(m => m.Page)
myFunction(loader)

Edit: even shorter:

declare function myFunction<U>(loader: P<(u: U) => void>): void;
interface P<V> {
    then<T1, T2>(pass: ((value: V) => T1), fail?: ((reason: any) => T2)): P<T1 | T2>;
}
declare var pp: P<(x: {}) => void>

myFunction(pp.then(m => m))

@sandersn
Copy link
Member

sandersn commented May 22, 2019

@ahejlsberg tracked this down a sentinel type, silentNever, possibly incorrectly escaping inference.

@ahejlsberg do you want to take over on this bug or do you want me to finish it up?

@ahejlsberg
Copy link
Member

Even smaller:

interface Prom<T> {
    then<T1, T2>(pass?: ((value: T) => T1), fail?: ((reason: any) => T2)): Prom<T1 | T2>;
}

declare function myFunction<T>(loader: () => Prom<(x: T) => void>): T;

declare let pp: Prom<(x: number) => void>;

const loader: () => Prom<(x: number) => void> = () => pp.then(m => m);

myFunction(() => pp.then(m => m));  // Error

@sandersn
Copy link
Member

I bisected the regression down to 1f32139 in #29847, which infers to unions containing multiple naked type variables. That improvement proceeds to uncover this bug.

@ahejlsberg
Copy link
Member

@sandersn Indeed, but the change in #29847 is entirely reasonable and fixes another issue related to promises. The real question here is where it goes wrong when we're making inferences through the generic return type of Promise.then.

@sandersn
Copy link
Member

Yep, no question #29847 is an improvement. This bug is just exposed by it, I think.

Here's what happens in a bit more detail:

when inferring type arguments for pp.then(m => m), we get a contextual type during inference for the outer myFunction(pp.then(m => m)) call resolution. We instantiate this type with the special inference flag NoDefault, which produces silentNever when there are no inferences (although it is documented to produce 'unknownType', which doesn't seem right):

const outerMapper = getMapperFromContext(cloneInferenceContext(getInferenceContext(node), InferenceFlags.NoDefault));
const instantiatedType = instantiateType(contextualType, outerMapper);

After that, we are inferring from (u: silentNever) => void to T1 | T2.

Removing NoDefault from getMapperFromContext fixes this example. I'm running tests to see what fails and learn why it is set.

@sandersn
Copy link
Member

There are two classes of test failures:

  1. promiseType and promiseTypeStrictNull: Fixes a bunch of silentNevers that were incorrectly baselined.
  2. inferFromGenericFunctionReturnTypes1 and 2: Breaks inference from generic function return types in many cases.

I think this means that the bug was technically caught two years ago in #13487, when we tweaked the definition of Promise and incorrectly accepted baselines. We just didn't notice it among the 1500 lines of churn for promiseType.ts.

@sandersn
Copy link
Member

@ahejlsberg I'm not sure if the correct fix is to change NoDefault to return silentNeverType less often or to change inference downstream to skip silentNever in more cases. Opinions? I think the second is probably a smaller change, so I'll look at that first.

@sandersn
Copy link
Member

I tried returning nonInferrableType since I believe I want propagation of non-inferrability, but it didn't actually propagate through anonymous type instantiation. I added that and then I get a different error:

error TS2345: Argument of type 'P<unknown>' is not assignable to parameter of type 'P<(u: unknown) => void>'.
  Type 'unknown' is not assignable to type '(u: unknown) => void'.

myFunction(pp.then(m => m))
           ~~~~~~~~~~~~~~~

It looks like I've prevented any inference from happening, even the good one from the first parameter of then.
This also breaks inferFromGenericFunctionReturnTypes1,2,3 tests, unsurprisingly.

@ahejlsberg Do you think that switching NoDefault to return nonInferrableType is the right idea? I feel like I've just made a mistake in the way I'm propagating it in instantiation.

@sandersn
Copy link
Member

Moving to 3.6 since we are out of time on 3.5, and it's not affecting a large number of people (and there's an easy workaround or two).

@sandersn sandersn modified the milestones: TypeScript 3.6.0, Backlog Jul 3, 2019
@sandersn sandersn removed their assignment Jan 7, 2020
@wilsoncook
Copy link

Any progress?

@jantimon
Copy link

jantimon commented May 5, 2021

@sandersn those workarounds can't be used by library writers but have to be known by the consumers of the library..

it is affecting a large number of people - e.g. everyone who is using nextjs with dynamic imports: https://nextjs.org/docs/advanced-features/dynamic-import

could you please consider picking up this issue once again?

@ctoth
Copy link

ctoth commented Jul 20, 2021

Was lead here experiencing this exact issue. Nothing really useful to say other than that.

@ibanlopez
Copy link

Moving to 3.6 since we are out of time on 3.5, and it's not affecting a large number of people (and there's an easy workaround or two).

Is still moving from 3.6 up to 5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants