GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Support LibraryManagedAttributes<TComponent, TAttributes> JSX namespace type by weswigham · Pull Request #24422 · microsoft/TypeScript

Conversation

@weswigham

@weswigham Wesley Wigham (weswigham) commented May 25, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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. Daniel Rosenwasser (@DanielRosenwasser) you probably have some opinions here, right? 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codepunkt, s-ve, ColCh, krinoid, thetrompf, arusahni, decademoon, pradel, guilhermebruzzi, MOZGIII, and 30 more reacted with thumbs up emoji demensky and franklixuefei reacted with thumbs down emoji s-ve, blainekasten, bradleyayers, brieb, mvestergaard, krinoid, thetrompf, arusahni, ladas-larry, AzkiyOne, and 8 more reacted with hooray emoji ColCh, arusahni, ladas-larry, kaoDev, ibezkrovnyi, benjaminRomano, damaon, goldbullet, SergioMorchon, krinoid, and 6 more reacted with heart emoji
All reactions
  • 👍 40 reactions
  • 👎 2 reactions
  • 🎉 18 reactions
  • ❤️ 16 reactions
Wesley Wigham (weswigham) and others added 3 commits May 25, 2018 11:01
@weswigham
WIP
9b5be9b
@weswigham
Allow type alias for managed type
88be791
@weswigham
Add a large test
085778a
@weswigham
Wesley Wigham (weswigham) requested a review from Daniel Rosenwasser (DanielRosenwasser) May 25, 2018 21:29
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented May 31, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed May 31, 2018
View reviewed changes
Comment thread tests/baselines/reference/tsxLibraryManagedAttributes.errors.txt Outdated
& {[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] : {}};

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 31, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jaredpalmer
Jared Palmer (jaredpalmer) reviewed Jun 6, 2018
View reviewed changes
Comment thread tests/baselines/reference/tsxLibraryManagedAttributes.js
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

@jaredpalmer Jared Palmer (jaredpalmer) Jun 6, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jaredpalmer

Jared Palmer (jaredpalmer) commented Jun 6, 2018

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@weswigham

Wesley Wigham (weswigham) commented Jun 6, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Jared Palmer (@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).

styfle, sywka, levenleven, piotrwitek, MichalLytek, rozzzly, IllusionMH, kohlmannj, cliedeman, epidemian, and 9 more reacted with thumbs up emoji
All reactions
  • 👍 19 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jaredpalmer

Jared Palmer (jaredpalmer) commented Jun 6, 2018

Copy link
Copy Markdown

got it so it will work like Flow. cool!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Hotell

Martin Hochel (Hotell) commented Jun 6, 2018

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ferdaber
Ferdy Budhidharma (ferdaber) reviewed Jun 7, 2018
View reviewed changes
Comment thread tests/cases/conformance/jsx/tsxLibraryManagedAttributes.tsx
type Defaultize<TProps, TDefaults> =
& {[K in Extract<keyof TProps, keyof TDefaults>]?: TProps[K]}
& {[K in Exclude<keyof TProps, keyof TDefaults>]: TProps[K]}
& Partial<TDefaults>;

@ferdaber Ferdy Budhidharma (ferdaber) Jun 7, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sylvanaar Jon S Akhtar (sylvanaar) mentioned this pull request Jun 11, 2018
Helper to derive React component props types from mapStateToProps / mapDispatchToProps piotrwitek/react-redux-typescript-guide#81
Closed
Wesley Wigham (weswigham) added 3 commits June 29, 2018 17:10
@weswigham
Merge branch 'master' into jsx-namespace-thing
a40833f
@weswigham
Accept updatedbaselines
ec40710
@weswigham
Fix typo in test, add one more example
8f62ada
@weswigham

Wesley Wigham (weswigham) commented Jun 30, 2018

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) I've synchronized this branch with master; can I get this reviewed?

dsifford, matijagrcic, krinoid, pbeshai, and hahnlee reacted with heart emoji
All reactions
  • ❤️ 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhegazy
Mohamed Hegazy (mhegazy) approved these changes Jun 30, 2018
View reviewed changes
@mhegazy

Mohamed Hegazy (mhegazy) commented Jun 30, 2018

Copy link
Copy Markdown
Contributor

We will need to update https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/JSX.md

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@weswigham
Wesley Wigham (weswigham) merged commit 18e3f48 into microsoft:master Jun 30, 2018
@OliverJAsh

Oliver Joseph Ash (OliverJAsh) commented Jul 6, 2018

Copy link
Copy Markdown
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" />;
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

Oliver Joseph Ash (@OliverJAsh) this is a different issue i am afraid.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@OliverJAsh

Oliver Joseph Ash (OliverJAsh) commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy

Mohamed Hegazy (mhegazy) commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

yes please.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@weswigham

Wesley Wigham (weswigham) commented Jul 6, 2018

Copy link
Copy Markdown
Member Author

Oliver Joseph Ash (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@weswigham
Wesley Wigham (weswigham) deleted the jsx-namespace-thing branch July 6, 2018 20:05
@OliverJAsh Oliver Joseph Ash (OliverJAsh) mentioned this pull request Jul 8, 2018
React component props: mixing defaultProps and union types #25503
Closed
@OliverJAsh

Oliver Joseph Ash (OliverJAsh) commented Jul 8, 2018

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) #25503.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Hotell Martin Hochel (Hotell) mentioned this pull request Jul 8, 2018
WIP: [react] more strict typings with breaking changes DefinitelyTyped/DefinitelyTyped#26956
Closed
6 of 9 tasks
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jul 9, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser left review comments
+3 more reviewers
@jaredpalmer Jared Palmer (jaredpalmer) jaredpalmer left review comments
@ferdaber Ferdy Budhidharma (ferdaber) ferdaber left review comments
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@weswigham @DanielRosenwasser @jaredpalmer @Hotell @mhegazy @OliverJAsh @ferdaber
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.