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

react-redux: conditional type inference from alias fails after re-alias support in #42284 #42421

Closed
sandersn opened this issue Jan 20, 2021 · 6 comments
Assignees
Labels
Breaking Change Would introduce errors in existing code Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@sandersn
Copy link
Member

Bug Report

On DT, react-redux/react-redux-tests.tsx:1433:

interface OwnProps {
    own: string;
}
const Component: React.FC<OwnProps & ReduxProps> = ({ own, dispatch }) => null;
                                                        // ~~~~~~~~ 'dispatch' does not exist on PropsWithChildren<OwnProps> 
const connector = connect();
type ReduxProps = ConnectedProps<typeof connector>;

🙂 Expected behavior

The object type that's the first parameter to the arrow function should have two properties own (from OwnProps) and dispatch (from ConnectedProps<typeof connector>.

ConnectedProps is a conditional type with a false branch never, and it looks like the false branch is incorrectly selected:

export type ConnectedProps<TConnector> =
    TConnector extends InferableComponentEnhancerWithProps<infer TInjectedProps, any>
        ? TInjectedProps
        : never;

That must be because inference to InferableComponentEnhancerWithProps<infer TInjectedProps, any> fails.

🙁 Actual behavior

Component: React.FC<OwnProps> not : React.FC<OwnProps & DispatchProp>

Workaround

I observed that connect: Connect and also that

interface Connect {
  (): InferableComponentEnhancer<DispatchProp>;
  // ... many other properties ...
}
export type InferableComponentEnhancer<TInjectedProps> =
    InferableComponentEnhancerWithProps<TInjectedProps, {}>;

Manually de-aliasing InferableComponentEnhancer fixes the error:

interface Connect {
  (): InferableComponentEnhancerWithProps<DispatchProp, {}>;
  // ... many other properties ...
}
@sandersn sandersn added this to the TypeScript 4.2.1 milestone Jan 20, 2021
@sandersn sandersn added the Bug A bug in TypeScript label Jan 20, 2021
@sandersn sandersn changed the title react-redux: conditional type inference to alias fails after re-alias support in #42284 react-redux: conditional type inference from alias fails after re-alias support in #42284 Jan 20, 2021
@ahejlsberg
Copy link
Member

The core issue here is that with our improved type alias preservation we now give distinct identities to otherwise equivalent instantiations of InferableComponentEnhancer and InferableComponentEnhancerWithProps. That in turn means we end up doing structural inference where otherwise we would have just inferred between type arguments--and the structural inference in this case is less precise because we erase type parameters from generic signatures before inferring.

It's possible we could improve inference between instantiated signatures that originate in the same signature declaration and fix the issue that way, but I'm not too hopeful.

@ahejlsberg
Copy link
Member

Looking a little deeper, the issue is that InferableComponentEnhancerWithProps uses the first type parameter in a lossy fashion that makes it impossible to recover during structural inference. So, only when inferring from the exact same alias can the information be recovered. The quickest fix right now would be to try inferring from the other alias when inference from the first alias fails:

export type ConnectedProps<TConnector> =
    TConnector extends InferableComponentEnhancerWithProps<infer TInjectedProps, any> ?
        unknown extends TInjectedProps ?
            TConnector extends InferableComponentEnhancer<infer TInjectedProps> ?
                TInjectedProps :
                never :
            TInjectedProps :
        never;

This fixes the issue and works in a backwards compatible matter.

sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Jan 21, 2021
Typescript 4.2 breaks react-redux' types in a subtle way, described at
microsoft/TypeScript#42421. That changes preserves type aliases, but it
also treats them more nominally for the purposes of inference.

The result is that ConnectedProps, which normally tries to infer a type
argument from InferableComponentEnhancerWithProps, needs to add a
fallback inference from InferableComponentEnhancer, *even though* the
latter is just a type alias.

This is a non-breaking change; it would be simpler just to remove
InferableComponentEnhancer and give InferableComponentEnhancerWithProps
a type parameter default, but that would be a breaking change since both
types are exported.
@sandersn
Copy link
Member Author

I put up a workaround PR for react-redux types.

sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Jan 22, 2021
Typescript 4.2 breaks react-redux' types in a subtle way, described at
microsoft/TypeScript#42421. That changes preserves type aliases, but it
also treats them more nominally for the purposes of inference.

The result is that ConnectedProps, which normally tries to infer a type
argument from InferableComponentEnhancerWithProps, needs to add a
fallback inference from InferableComponentEnhancer, *even though* the
latter is just a type alias.

This is a non-breaking change; it would be simpler just to remove
InferableComponentEnhancer and give InferableComponentEnhancerWithProps
a type parameter default, but that would be a breaking change since both
types are exported.
@ahejlsberg ahejlsberg added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Jan 22, 2021
@ahejlsberg ahejlsberg removed this from the TypeScript 4.2.1 milestone Jan 22, 2021
@ahejlsberg
Copy link
Member

@DanielRosenwasser We should include this in the release notes as a scenario that might be broken by #42284.

@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Jan 22, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.2.0 milestone Jan 22, 2021
@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@DanielRosenwasser
Copy link
Member

I understand what is happening, but I don't know if I can reliably communicate that in the release notes for the average user though. I'll think on it.

kaznovac pushed a commit to kaznovac/DefinitelyTyped that referenced this issue Mar 2, 2021
Typescript 4.2 breaks react-redux' types in a subtle way, described at
microsoft/TypeScript#42421. That changes preserves type aliases, but it
also treats them more nominally for the purposes of inference.

The result is that ConnectedProps, which normally tries to infer a type
argument from InferableComponentEnhancerWithProps, needs to add a
fallback inference from InferableComponentEnhancer, *even though* the
latter is just a type alias.

This is a non-breaking change; it would be simpler just to remove
InferableComponentEnhancer and give InferableComponentEnhancerWithProps
a type parameter default, but that would be a breaking change since both
types are exported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants