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

Generic object rest variables and parameters #28312

Merged
merged 14 commits into from
Nov 6, 2018
Merged

Generic object rest variables and parameters #28312

merged 14 commits into from
Nov 6, 2018

Conversation

ahejlsberg
Copy link
Member

With this PR we permit rest properties in destructurings of objects of generic types. This effectively implements what is suggested in #10727 (although by different means) and complements our support for generic spread expressions in object literals implemented in #28234.

When a destructuring of an object of a generic type includes a rest variable, the type of the rest variable is an instantiation of the Pick and Exclude predefined types:

function excludeTag<T extends { tag: string }>(obj: T) {
    let { tag, ...rest } = obj;
    return rest;  // Pick<T, Exclude<keyof T, "tag">>
}

const taggedPoint = { x: 10, y: 20, tag: "point" };
const point = excludeTag(taggedPoint);  // { x: number, y: number }

Some additional examples:

const sa = Symbol();
const sb = Symbol();

function f1<T extends { [sa]: string, [sb]: number }>(obj: T) {
    let { [sa]: a, [sb]: b, ...rest } = obj;
    return rest;  // Pick<T, Exclude<keyof T, typeof sa | typeof sb>>
}

function f2<T, KA extends keyof T, KB extends keyof T>(obj: T, ka: KA, kb: KB) {
    let { [ka]: a, [kb]: b, ...rest } = obj;
    return rest;  // Pick<T, Exclude<keyof T, KA | KB>>
}

@benjaminjackman
Copy link

Would it be worth considering adding type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>> as a predefined type and using that here?

@ahejlsberg
Copy link
Member Author

Would it be worth considering adding type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>> as a predefined type and using that here?

We considered that, but there are too many projects out there that already define their own Omit and we'd end up with duplicate definition issues.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a noLib test would be nice, but it looks good.

@@ -34,8 +33,6 @@ tests/cases/conformance/types/rest/objectRestNegative.ts(17,9): error TS2701: Th
~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHould probably remove this function generic<T ... example since the new test covers exactly the same case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but no harm in keeping it.

interface I {
[s: string]: boolean;
[s: number]: boolean;
}

var o: I = {
~
!!! error TS2322: Type '{ [x: string]: string | number; }' is not assignable to type 'I'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea what's going on here? I'm not sure why we stopped issuing the per-property error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed it was the change in looking up the type of property names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because we previously treated all computed properties as having known (unit type) names, when here they actually have type string. Since they now have type string we generate a string index signature, and the elaboration logic stops trying to drill down to the properties.

# Conflicts:
#	src/compiler/checker.ts
@ahejlsberg ahejlsberg merged commit 85dbc04 into master Nov 6, 2018
@ahejlsberg ahejlsberg deleted the genericRest branch November 6, 2018 00:36
@vkrol
Copy link

vkrol commented Nov 19, 2018

Is this scenario is not supported? Or this is a bug?

function foo<T>(obj: T & { x: string }): T {
    const { x, ...rest } = obj;
    return rest; // Error: [ts] Type 'Pick<T & { x: string; }, Exclude<keyof T, "x">>' is not assignable to type 'T'. [2322]
}

@ajafff
Copy link
Contributor

ajafff commented Nov 19, 2018

@vkrol if T contains a property x it will not be present after the destructuring. Therefore the error is correct.

@vkrol
Copy link

vkrol commented Nov 19, 2018

@ajafff But T does not contains x, T & { x: string } contains x 🤔.

@sandersn
Copy link
Member

@vkrol Let T={ x: number }. Now T contains x.

@ajafff While this is correct, I think it’s actually undesired. Most of the motivation for the higher-order definition of spread as intersection was that it’s quite usable, as well as correct [1] for the common case where two disjoint types are spread together (the case here) or the same type is spread into itself.

The definition of rest we ended up with is actually more correct, as you note, but it means that it doesn’t have the same assignability behaviour as spread. I think the correct fix here would be to add a simplification rule in assignability checking that says that Pick<T & U, Exclude<keyof T, keyof U>> is assignable to T.

However, I’m not sure if such a rule would be applicable anywhere else, and I think it would be fiddly to get right since U and keyof U are not tightly linked, so you’d have to relate them via assignability instead of just checking type equality.

[1] except of course for own-ness, which the compiler doesn’t track well anyway.

@Hotell
Copy link

Hotell commented Nov 19, 2018

What about following scenario @sandersn ( related to react HoC, but the same behaviour happens with raw functions )

image

Error:
image

Fix:

It needs to be casted to original P to get rid of errors, although InjectedProps are subtracted from P constraint within the implementation and the exact shape is properly passed via const injectedProps

image

So is this correct behaviour or a bug ?

@sandersn
Copy link
Member

@Hotell

As far as I understand the problem, I think it's the same as @vkrol's. I'm not sure I understand it, however.

If InjectedProps is the type that's related to withCounter, why does counterWannabe have to declare it as a type at all? Shouldn't extendedFunc be the one with the dependency on the counter-related types InjectedProps and ExtendedProps?

In other words, why does P extend InjectedProps? Shouldn't it be unconstrained and then innerFunc be (props: P & ExtendedProps) => P & InjectedProps ?

Of course, given the const { maxCount, ...passThroughProps } = props and the problem @vkrol lays out above, you'll get Pick<P & ExtendedProps, Exclude<keyof P & ExtendedProps, keyof ExtendedProps>> instead of P for the type of passThroughProps.

@Hotell
Copy link

Hotell commented Nov 20, 2018

@sandersn I wanted to simplify the example by not involving react but I guess that was bad move :D sorry about that. So here is the React HoC example, with comments:

update 11/22/18:

  • added InjectedProps definition
  • passThroughProps are casted to P -> {...passThroughProps as P}
import React, { Component } from 'react'

import { Counter } from './counter-render-prop'
import { Subtract } from '../types'

type ExtractFuncArguments<T> = T extends (...args: infer A) => any ? A : never;

// get props that Counter injects via children as a function
// InjectedProps is gonna be:
// { count: number; } & { inc: () => void; dec: () => void; }
type InjectedProps = ExtractFuncArguments<Counter['props']['children']>[0];

// withCounter will enhance returned component by ExtendedProps
type ExtendedProps = { maxCount?: number };

// P is constrained to InjectedProps as we wanna make sure that wrapped component
// implements this props API
const withCounter = <P extends InjectedProps>(Cmp: React.ComponentType<P>) => {
  class WithCounter extends Component<
    // enhanced component will not include InjectedProps anymore as they are injected within render of this HoC and API surface is gonna be extended by ExtendedProps
    Subtract<P, InjectedProps> & ExtendedProps
  > {
    static displayName = `WithCounter(${Cmp.name})`;

    render() {
      const { maxCount, ...passThroughProps } = this.props;

      return (
       // we use Counter which has children as a function API for injecting props
        <Counter>
          {(injectedProps) =>
            maxCount && injectedProps.count >= maxCount ? (
              <p className="alert alert-danger">
                You've reached maximum count! GO HOME {maxCount}
              </p>
            ) : (
              // here cast to as P is needed otherwise compile error will occur
              <Cmp {...injectedProps} {...passThroughProps as P} />
            )
          }
        </Counter>
      );
    }
  }

  return WithCounter;
};

// CounterWannabe implement InjectedProps on it's props
class CounterWannabe extends Component<
  InjectedProps & { colorType?: 'primary' | 'secondary' | 'success' }
> {
  render() {
    const { count, inc, colorType } = this.props;

    const cssClass = `alert alert-${colorType}`;

    return (
      <div style={{ cursor: 'pointer' }} className={cssClass} onClick={inc}>
        {count}
      </div>
    );
  }
}

// if CounterWannabe would not implement InjectedProps this line would get compile error
const ExtendedComponent = withCounter(CounterWannabe);

@sandersn
Copy link
Member

@Hotell What's the source of Counter? I'm not that familiar with React, so I can't guess how the children of Counter are supposed to be typed.

@sandersn
Copy link
Member

I read the intro to HOCs and I think I understand what's going on, although I haven't had a chance to play with the whole example without Counter.

  1. I still think this is the same issue as posted @vkrol encountered. That is, Rest<'a', T & { a }> should be assignable to T. I'm not sure why casting injectedProps is needed instead of passThroughProps though.
  2. Can't you spread <Cmp {...injectedProps} {...this.props} /> instead? It looks to me the only reason that normal React HOCs use passThroughProps is so that the passthrough component doesn't access the HOC's private state by mistake. But this is not likely in Typescript, because the type checker will complain if CounterWannabe accesses maxCount.
  3. Let's file a new issue for this: Generic rest of intersection should be assignable to its type parameter constituents #28636. I think it's a real hole in type simplifications of today.

@Hotell
Copy link

Hotell commented Nov 22, 2018

What's the source of Counter?

Sorry about that, it's not really important I should explicitly provide props in that example.

👉 I've updated previous comment

type State = typeof initialState
type Props = {
  children: (
    props: State & { inc: Counter['handleInc']; dec: Counter['handleDec'] }
  ) => React.ReactChild
  count?: number
} & typeof defaultProps

const initialState = { count: 0 }
const defaultProps = {
  onChange: (value: number) => {}
}
export class Counter extends Component<Props, State> {
  static defaultProps = defaultProps
  render(){ /*...*/ }
}

I'm not sure why casting injectedProps is needed instead of passThroughProps though.

My bad, updated the code above

Can't you spread <Cmp {...injectedProps} {...this.props} /> instead?

will produce the same error

image

let's file a new issue for this: #28636.
I agree, can you update the code in that issue pls with my fixes ?

Thanks a lot @sandersn 💪

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

Successfully merging this pull request may close these issues.

7 participants