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

Suggestion: type checking for JSX children #13618

Closed
jwbay opened this issue Jan 21, 2017 · 27 comments
Closed

Suggestion: type checking for JSX children #13618

jwbay opened this issue Jan 21, 2017 · 27 comments
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@jwbay
Copy link
Contributor

jwbay commented Jan 21, 2017

Rationale

Props are an important part of a React component's interface, but children can be as well.
In applications it's not uncommon to want to restrict what kind of children a component will
accept. Currently this enforcement has to be done at runtime by throwing errors.

The Goal

Components can specify allowed children as a union type. Each member would correspond to
the tag, or 'type' field, of the resulting Element. Ideally this could be plucked from
the type of props.children, which would satisfy both component classes and SFCs.

Motivating Examples:

A modal component may want to make assumptions about its children to satisfy layout
constraints.

interface IModalProps {
    children: ModalHeader | ModalBody | ModalFooter;
}

function Modal({ children }: IModalProps) { ... }
function ModalHeader(props) { ... }
function ModalBody(props) { ... }
function ModalFooter(props) { ... }

<Modal>
    <ModalHeader />
    <div /> { /* Desired error: type 'div' does not exist in type 'ModalHeader | ModalBody | ModalFooter' */ }
    <ModalBody />
    <ModalFooter />
</Modal>

Similarly...

interface IButtonGroupProps {
    children: 'button';
}

function ButtonGroup({ children }: IButtonGroupProps) { ... }

<ButtonGroup>
    <button />
    <button />
    <a href='' /> { /* Desired error: type 'a' is not assignable to type 'button' */ }
</ButtonGroup>

An interesting emerging pattern is using JSX as a function call to turn imperative APIs into
declarative ones. Currently these 'render callbacks' can't be type-checked at all. A more complete
summary can be found at http://reactpatterns.com/#render-callback. A further example can be found at https://github.com/ReactTraining/react-media#usage.

interface IUser {
    Name: string;
}

interface IFetchUserProps {
    children(user: IUser): any;
}

class FetchUser extends React.Component<IFetchUserProps, any> {
    render() {
        return this.state
            ? this.props.children(this.state.result)
            : null;
    }

    componentDidMount() {
        api.fetchUser().then(result => this.setState({ result }));
    }
}

function UserName() {
    return (
        <FetchUser>
            { user => (
                <h1>{ user.NAme }</h1> // Desired error: property 'NAme' does not exist on type 'IUser'
            ) }
        </FetchUser>
    );
}

Lastly, I don't think any of this is necessarily specific to React. Other libraries leveraging the JSX
specification for component structure should be able to leverage this as well. The crux of this lies at
the relationship between the type of the 'children' attribute of a given tag and the children actually
passed to it at usage sites.

@joelday
Copy link
Contributor

joelday commented Jan 30, 2017

@jwbay @RyanCavanaugh Did a quick prototype of this: joelday@50870c1

I'm sure there are a bunch of little gotchas here, but it's a start.

    // A custom element type with an explicit children prop attribute type
    
    class MyParentClass extends React.Component<{
      children?: MyClass[];
    }, any> {
    }
    
    // OK - Child element matches the children prop
    var d1 = <MyParentClass><MyClass reqd={true} /><MyClass reqd={true}></MyClass></MyParentClass>
    // Error - Incorrect child element type
    var d2 = <MyParentClass><div /><div></div><MyClass reqd={true} /><MyClass reqd={true}></MyClass></MyParentClass>
                            ~~~~~~~
!!! error TS2322: Type 'HTMLProps<HTMLDivElement>' is not assignable to type 'MyClass'.
!!! error TS2322:   Property 'setState' is missing in type 'HTMLProps<HTMLDivElement>'.
                                   ~~~~~
!!! error TS2322: Type 'HTMLProps<HTMLDivElement>' is not assignable to type 'MyClass'.```

@jwbay
Copy link
Contributor Author

jwbay commented Feb 2, 2017

@joelday Oh, nice! Many thanks for taking at look at an implementation!

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Feb 2, 2017
@mhegazy mhegazy added this to the TypeScript 2.3 milestone Feb 2, 2017
@joelday
Copy link
Contributor

joelday commented Feb 9, 2017

@yuit @mhegazy Let me know if there's anything I can do to help with this. Thank you!

@joelday
Copy link
Contributor

joelday commented Mar 29, 2017

@yuit Any traction with this? Would love for this to still make it into 2.3. Please let me know if there's anything I can do.

@petdun2519
Copy link

Would also like to see this implemented!

@yuit
Copy link
Contributor

yuit commented Apr 6, 2017

@joelday @petdun2519 PR will go up soon. Thanks for your protoype

@yuit yuit added the Fixed A PR has been merged for this issue label Apr 12, 2017
@Jessidhia
Copy link

Tangentially related, but I wonder if there is a way to enforce that the children props, internally, is accessed using the React.Children helpers (https://facebook.github.io/react/docs/react-api.html#react.children). The value of children, as seen from the component implementation, is supposed to be opaque until accessed with a helper.

@cchamberlain
Copy link

This change has broken quite a few things for a patch release. The errors are not that helpful that I'm seeing, tsc throws TS2710: 'children' are specified twice... on spread props. If I attempt to strip out a children prop and then pass the filtered props the editor shows squigglies indicating children does not exist on the type I'm trying to filter them out of.

@joscha
Copy link
Contributor

joscha commented May 10, 2017

This change has broken quite a few things for a patch release. The errors are not that helpful that I'm seeing, tsc throws TS2710: 'children' are specified twice... on spread props. If I attempt to strip out a children prop and then pass the filtered props the editor shows squigglies indicating children does not exist on the type I'm trying to filter them out of.

I've noticed this as well @cchamberlain , our code now looks like this:

const { children, ...rest } = this.props;
return <Bla {...rest}>{children}</Bla>;

@cchamberlain
Copy link

@joscha - That did work for me in some of the instances I tried but there was at least one spot that when I tried it, it complained that the variable I was spreading did not contain an element children. The element being spread was not {...this.props} directly, it was {...this.props.childProps} and did not have children defined on its interface. I reverted back to TS 2.3.0 so I don't have an exact representation at the moment unfortunately.

I know TypeScript can't follow traditional semver without making most releases major versions, but this change breaks every component using spread and children (majority of our components) so seems like it warrants a minor version.

@cchamberlain
Copy link

cchamberlain commented May 10, 2017

Don't want to beat a dead horse but I just installed VS 2017 update 15.2 per #14529 (comment) and it only allows selection of TypeScript minor version, meaning I have to revert all the way back to 2.2.x if breaking changes are released on a patch (VS is reporting the errors I get with TS 2.3.2 even though I have package.json / yarn locked at 2.3.0).

image

Update: I got my solution working on 2.3, we had many instances of children being typed as the wrong type, causing this update to throw many errors. I very much like the typed children support, just feel that if a TypeScript release is not meant to strictly patch a previous release it should get a minor version, especially if the only thing we can specify in VS 2017 is minor version.

@donaldpipowitch
Copy link
Contributor

Looking at #15160 (comment) it seems that the original idea of this issue isn't implemented and that the issue should be re-opened (imho).

You can type your children as

  • a string, boolean, etc.
  • to have no element, one element or multiple elements
  • to be a function (which is really nice)

But can't say that a <ButtonGroup/> can only have <Button/>'s or that <Modal/> can only have <ModalHeader/>, <ModalBody/> and <ModalFooter/> as children.

That's because <Button/>, <ModalHeader/> and so on return a generic JSX.Element type.

@joewood
Copy link

joewood commented Jun 14, 2017

@donaldpipowitch agreed, the current implementation doesn't seem to work as advertised.

@donaldpipowitch
Copy link
Contributor

The more I think of it I come to the conclusion that this could probably not be solved at a type checking level, because of the way TSX works currently. We can't use something like tagging JSX.Element:

enum FooterTag {}
type FooterElement = JSX.Element & FooterTag;
const Footer = () => <div>Footer.</div> as FooterElement;

interface ModalProps {
  children: FooterElement;
}
const Modal = ({ children }: ModalProps) =>
  <div>
    {children}
  </div>;

// `<Footer />` *needs* to be casted to `FooterElement`,
// because the return value is always `JSX.Element`
const UseModal = () =>
  <Modal>
    {<Footer /> as FooterElement}
  </Modal>;

When we useTSX the return value of our component will always be JSX.Element, because of the way it is transpiled. If we can't change this way, than the only solution would be to make this check inside a linter rule. With a linter rule we could easily say "Hey, you can only use <Footer /> inside <Modal />." or something like that. This would be possible, if #15589 could be integrated into the language.

@donaldpipowitch
Copy link
Contributor

donaldpipowitch commented Aug 8, 2017

How could this happen?

yuit was unassigned by donaldpipowitch 18 minutes ago

@antanas-arvasevicius
Copy link

antanas-arvasevicius commented Aug 8, 2017 via email

@RyanCavanaugh
Copy link
Member

@donaldpipowitch Yui's left the team but was still assigned the issue. GitHub fixes this invalid assignment whenever someone makes any change (incl. comment) on an issue

@jwbay
Copy link
Contributor Author

jwbay commented Aug 8, 2017

Couldn't JSX.Element be made generic with props/attributes?

const i = <input value='42' />;
//yields JSX.Element<{ value: string }>

const c = <Component prop={ 42 } />
//yields JSX.Element<{ prop: number }>

At this point the restricted children case devolves slightly from 'you can only pass something shaped like ModalFooter here' to 'you can only pass something with props shaped like ModalFooter's props', but for components I think that's really all that matters.

@donaldpipowitch
Copy link
Contributor

donaldpipowitch commented Aug 9, 2017

Yui's left the team but was still assigned the issue. GitHub fixes this invalid assignment whenever someone makes any change (incl. comment) on an issue

Ah, I see. Thanks. So if you ever need someone new (remote)... 😏

At this point the restricted children case devolves slightly from 'you can only pass something shaped like ModalFooter here' to 'you can only pass something with props shaped like ModalFooter's props', but for components I think that's really all that matters.

As far as I understand this issue and what I personally would like to see is the first case ("you can only pass something shaped like ModalFooter here"), not the second case ("you can only pass something with props shaped like ModalFooter's props").

I want to enforce ordering "rules" of components similar to native DOM element ordering "rules". E.g. from <colgroup>:

The <colgroup> must appear after any optional <caption> element but before any <thead>, <th>, <tbody>, <tfoot> and <tr> element.

@patsissons
Copy link

what has become of this issue, is it dead?

@donaldpipowitch
Copy link
Contributor

I don't want to generate noise for the maintainers, but maybe we should just open a new issue for a focussed discussion and re-evaluation of this feature...?

@gitsupersmecher
Copy link

something like this is a mitigation: export interface IMenuPorps { children ? : any; }

@dontsave
Copy link

dontsave commented Jan 6, 2018

Curious about the status of this issue as well. Being able to restrict children to specific types would be very useful

@luixo
Copy link

luixo commented Feb 1, 2018

Chances are I misunderstood the issue topic, but I have somehow related problem.
So I have some component with its props typed:

export interface MyProps {
    extraElement: React.ReactNode;
}

class MyInnerComponent extends React.Component<MyProps> {
    render(): React.ReactNode {
        return (
            <div>
                {this.props.extraElement}
                {this.props.children}
            </div>
        );
    }
}

export default MyInnerComponent;

Now I want to calculate children in another component.

import MyInnerComponent, {MyProps as InnerComponentProps} from '...';

class MyWrapperComponent extends React.Component<{}> {
    calculateProps(): InnerComponentProps {
        return {
            extraElement: <div>Hello!</div>,
            children: [<div>First</div>, <div>Second</div>]
        };
    }
    render(): React.ReactNode {
        return (
            <MyInnerComponent {...this.calculateProps()} />
        );
    }
}

In this I get TS2322: Type { extraElement: Element; children: Element[] } is not assignable to type 'MyProps'. Object literal may only specify known properties, and 'children' does not exist in type 'MyProps'.
Probably, I should extend MyProps with something that includes a proper check for children being React.ReactNode or something, but I didn't found anything that look like that in @types/react.

@jchitel
Copy link

jchitel commented Feb 13, 2018

Recent discussion in this issue seems to be about allowing the JSX.Element type to be generic, or at least having JSX typing be more flexible. These use cases are covered by this issue, which is still open. I'd suggest moving all conversation there so that we can move toward an implementable proposal.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 13, 2018

#21699 should cover this scenario.

@elbeg
Copy link

elbeg commented Mar 8, 2018

OK

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: JSX/TSX Relates to the JSX parser and emitter Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests