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鈥檒l 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

Projects
None yet
7 participants
@weswigham
Member

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? 馃槃

weswigham added some commits May 25, 2018

WIP

@weswigham weswigham requested a review from DanielRosenwasser May 25, 2018

@DanielRosenwasser

This comment has been minimized.

Show comment
Hide comment
@DanielRosenwasser

DanielRosenwasser May 31, 2018

Member

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

Member

DanielRosenwasser commented May 31, 2018

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

@jaredpalmer

This comment has been minimized.

Show comment
Hide comment
@jaredpalmer

jaredpalmer Jun 6, 2018

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

jaredpalmer commented Jun 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jun 6, 2018

Member

@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).

Member

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

This comment has been minimized.

Show comment
Hide comment
@jaredpalmer

jaredpalmer Jun 6, 2018

got it so it will work like Flow. cool!

jaredpalmer commented Jun 6, 2018

got it so it will work like Flow. cool!

@Hotell

This comment has been minimized.

Show comment
Hide comment
@Hotell

Hotell 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

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>;

This comment has been minimized.

@ferdaber

ferdaber Jun 7, 2018

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]?

@ferdaber

ferdaber Jun 7, 2018

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 added some commits Jun 30, 2018

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jun 30, 2018

Member

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

Member

weswigham commented Jun 30, 2018

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

@mhegazy

This comment has been minimized.

Show comment
Hide comment
Contributor

mhegazy commented Jun 30, 2018

@weswigham weswigham merged commit 18e3f48 into Microsoft:master Jun 30, 2018

5 checks passed

VSTS: node10 6230 succeeded
Details
VSTS: node6 6228 succeeded
Details
VSTS: node8 6229 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@OliverJAsh

This comment has been minimized.

Show comment
Hide comment
@OliverJAsh

OliverJAsh Jul 6, 2018

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" />;
}

OliverJAsh commented Jul 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jul 6, 2018

Contributor

@OliverJAsh this is a different issue i am afraid.

Contributor

mhegazy commented Jul 6, 2018

@OliverJAsh this is a different issue i am afraid.

@OliverJAsh

This comment has been minimized.

Show comment
Hide comment
@OliverJAsh

OliverJAsh Jul 6, 2018

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

OliverJAsh commented Jul 6, 2018

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

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jul 6, 2018

Contributor

yes please.

Contributor

mhegazy commented Jul 6, 2018

yes please.

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jul 6, 2018

Member

@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.

Member

weswigham commented Jul 6, 2018

@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.

@weswigham weswigham deleted the weswigham:jsx-namespace-thing branch Jul 6, 2018

@OliverJAsh

This comment has been minimized.

Show comment
Hide comment
@OliverJAsh

OliverJAsh commented Jul 8, 2018

@Hotell Hotell referenced this pull request Jul 8, 2018

Closed

WIP: [react] more strict typings with breaking changes #26956

6 of 9 tasks complete
@DanielRosenwasser

This comment has been minimized.

Show comment
Hide comment
@DanielRosenwasser

DanielRosenwasser Jul 9, 2018

Member

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

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.