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

Support LibraryManagedAttributes<TComponent, TAttributes> JSX namespace type #24422

Merged
merged 6 commits into from
Jun 30, 2018

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented May 25, 2018

Fixes #23812

Allows, eg:

class Component extends ReactComponent {
    static propTypes = {
        foo: PropTypes.number,
        bar: PropTypes.node,
        baz: PropTypes.string.isRequired,
    };
    static defaultProps = {
        foo: 42,
    }
}

const a = <Component foo={12} bar="yes" baz="yeah" />;
const b = <Component foo={12} />; // Error, missing required prop bar
const c = <Component bar="yes" baz="yeah" />;
const d = <Component bar="yes" baz="yo" bat="ohno" />; // Error, baz not a valid prop
const e = <Component foo={12} bar={null} baz="cool" />; // bar is nullable/undefinable since it's not marked `isRequired`
const f = <Component foo={12} bar="yeah" baz={null} />; // Error, baz is _not_ nullable/undefinable since it's marked `isRequired`

when JSX.LibraryManagedAttributes<TComponent, TAttributes> is appropriately defined. Thanks to conditional types, this method of supporting propTypes and defaultProps allows definition authors a ton of freedom in how the static types need to be transformed before they are checked against what the user wrote. (Allowing customization of, for example: how conflicts between provided props and inferred props are handled, how inferences are mapped, how optionality is handled, and how inferences from differing places should be combined)

As an aside, you can fully implement the functionality of both InstrinsicAttributes and InstrinsicClassAttributes using this new type entrypoint, so in a way it obsoletes them.

Feel free to 🚲 🏠 with the name and give feedback; the actual implementation is relatively simple. @DanielRosenwasser you probably have some opinions here, right? 😄

@DanielRosenwasser
Copy link
Member

Could you elaborate in this PR on what LibraryManagedAttributes is intended to do and give some specific implementations examples?

& {[K in Exclude<keyof TProps, keyof TDefaults>]: TProps[K]}
& Partial<TDefaults>;

type InferedPropTypes<P> = {[K in keyof P]: P[K] extends PropTypeChecker<infer T, infer U> ? PropTypeChecker<T, U>[typeof checkedType] : {}};
Copy link
Member

Choose a reason for hiding this comment

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

I know it's just a test, but InferedPropTypes -> InferredPropTypes

const x = <JustDefaultPropsWithSpecifiedGeneric foo="eh" />;
const y = <JustDefaultPropsWithSpecifiedGeneric foo="no" bar="ok" />; // error, no prop named bar
const z = <JustDefaultPropsWithSpecifiedGeneric foo={12} />; // error, wrong type

Choose a reason for hiding this comment

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

What about <JustDefaultPropsWithSpecifiedGeneric />? Is that kosher?

@jaredpalmer
Copy link

As background, TS works differently than Flow at the moment with React defaultProps, often causing pain and confusion among junior devs. With this PR, defaultProps will be intuitive. I (think) the ideal behavior is this:

Before

interface Props {
  thing?: string; // thing is optional, but it is always defined because it has a default prop.
}

class Thing extends React.Component<Props> {
   static defaultProps = {
      thing: 'hello'
   }

  render() {
     console.log(this.props.thing!.length) // forced with !
    // ...
 }
}

// ..

<Thing /> // this works

After

interface Props {
  thing?: string; // still optional
}

class Thing extends React.Component<Props> {
   static defaultProps = {
      thing: 'hello'
   }

  render() {
    console.log(this.props.thing) // inferred because of defaultProps, no ! needed
    // ...
 }
}
// ..
<Thing /> // works
@weswigham
Copy link
Member Author

weswigham commented Jun 6, 2018

@jaredpalmer The Props type parameter should be the "internal" props (so what you read), while the props returned by the type alias should be the "external" props (the ones you write in a tag). So, instead of

interface Props {
  thing?: string; // still optional
}

class Thing extends React.Component<Props> {
   static defaultProps = {
      thing: 'hello'
   }

  render() {
    console.log(this.props.thing) // inferred because of defaultProps, no ! needed
    // ...
 }
}
// ..
<Thing /> // works

you'd have

interface Props {
  thing: string; // note: not optional within class, but is optional when writing due to the default
}

class Thing extends React.Component<Props> {
   static defaultProps = {
      thing: 'hello'
   }

  render() {
    console.log(this.props.thing) // strict type from `Props`
    // ...
 }
}
// ..
<Thing /> // works, thanks to `defaultProps`

or at least that's what makes sense to me - since the Props generic param is the type of the first argument to the factory/class (which includes all defaults).

@jaredpalmer
Copy link

got it so it will work like Flow. cool!

@Hotell
Copy link

Hotell commented Jun 6, 2018

This is indeed awesome! In the meantime I wrote an article for React consumers struggling with this "deal breaker" ✌️ https://medium.com/@martin_hotell/react-typescript-and-defaultprops-dilemma-ca7f81c661c7

type Defaultize<TProps, TDefaults> =
& {[K in Extract<keyof TProps, keyof TDefaults>]?: TProps[K]}
& {[K in Exclude<keyof TProps, keyof TDefaults>]: TProps[K]}
& Partial<TDefaults>;
Copy link

Choose a reason for hiding this comment

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

Why is the Partial<TDefaults> here intersected with the extracted mapped type from TDefaults? What kind of error does that produce if typeof TProps[K] is incompatible with typeof TDefaults[K]?

@weswigham
Copy link
Member Author

@mhegazy I've synchronized this branch with master; can I get this reviewed?

@weswigham weswigham merged commit 18e3f48 into microsoft:master Jun 30, 2018
@OliverJAsh
Copy link
Contributor

What is the expected behaviour when the props type is a union? Is this expected?

{
    type Props = { shared: string } & (
        | {
              type: 'foo';
              foo: string;
          }
        | {
              type: 'bar';
              bar: string;
          });
    class Component extends ReactComponent<Props> {
        static defaultProps = {
            shared: 'yay',
        };
    }

    <Component type="foo" />; // Expected error, got none
    // Unexpected error
    // Property 'foo' does not exist on type 'Defaultize<Props, { shared: string; }>'.
    <Component type="foo" foo="1" />;
}
@mhegazy
Copy link
Contributor

mhegazy commented Jul 6, 2018

@OliverJAsh this is a different issue i am afraid.

@OliverJAsh
Copy link
Contributor

@mhegazy Is it an issue worth tracking? If so, I'll open a separate issue.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 6, 2018

yes please.

@weswigham
Copy link
Member Author

@OliverJAsh it's probably also worth noting that while this code is in typescript, there won't be any changes until the react .d.ts is updated to use the new jsx namespace entrypoint; so until that happens defaultProps still won't do anything.

@OliverJAsh
Copy link
Contributor

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 9, 2018

Locking so users open up new issues instead. That way, we can more easily follow up.

@microsoft microsoft locked as resolved and limited conversation to collaborators Jul 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
7 participants