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

[Master] Type checking JSX children #15160

Merged
merged 24 commits into from
Apr 21, 2017
Merged

[Master] Type checking JSX children #15160

merged 24 commits into from
Apr 21, 2017

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Apr 12, 2017

Fix #13618 : type checking children property (for examples and motivation see the original issue) Thanks @joelday for prototyping the feature!

Details

  • Look for "props" in JSX.ElementChildrenAttribute if there exist a property inside(by default in react.d.ts it is children) that property name will be used as a name of children property. If such property doesn't exist (e.g using older react.d.ts), we will just check all the children but will NOT attach the type as "children" property.

  • If there existed children in the body of JSX expression, synthesize symbol presenting those children and add them as part of JSX attributes named children. If there already children specified as attribute of opening element there will be an error indicating that the attribute will get overwritten.

  • When we check each child, we call into checkExpression what this mean is that all JSX Expression will have type JSX.Element. In the future, by treating as JSX.Element will allow us to easily extends for doing these works [JSX] Cannot declare JSX elements to be in specific types. #13746 or Type JSX elements based on createElement function #14729.....

Examples

interface Prop {
    a: number,
    b: string,
    children: string | JSX.Element
}

function Comp(p: Prop) {
    return <div>{p.b}</div>;
}

// Error: missing children
let k = <Comp a={10} b="hi" />;
// Error: type not match
// For this to work, "children: (string | JSX.Element)[]"
let k1 = <Comp a={10} b="hi">
    <h2>Hello</h2> "hi"
</Comp>

// OK
let k1 = <Comp a={10} b="hi">
    <h2>Hello</h2>
</Comp>
;
interface IUser {
    Name: string;
}

interface IFetchUserProps {
    children: (user: IUser) => JSX.Element;
}

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

// Error
function UserName() {
    return (
        <FetchUser>
            { user => (
                <h1>{ user.NAme }</h1>   // This is actually an error now; see. tests/baselines/reference/checkJsxChildrenProperty4.errors.txt
            ) }
        </FetchUser>
    );
}

Note: with the <Fetchuser> example, we can now provide contextual type and give completion as well (see fourslash test)

@@ -424,6 +424,8 @@ namespace ts {
IntrinsicClassAttributes: "IntrinsicClassAttributes"
};

const jsxChildrenPropertyName = "children";
Copy link
Contributor

Choose a reason for hiding this comment

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

i would say we should treat this the same way we do with props instead of hard coding it.

@@ -8676,8 +8680,9 @@ namespace ts {
function hasExcessProperties(source: FreshObjectLiteralType, target: Type, reportErrors: boolean): boolean {
if (maybeTypeOfKind(target, TypeFlags.Object) && !(getObjectFlags(target) & ObjectFlags.ObjectLiteralPatternWithComputedProperties)) {
const isComparingJsxAttributes = !!(source.flags & TypeFlags.JsxAttributes);
const containsSynthesizedJsxChildren = !!(source.flags & TypeFlags.ContainsSynthesizedJsxChildren);
Copy link
Contributor

Choose a reason for hiding this comment

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

do not think we need this here

@@ -2099,6 +2099,10 @@
"category": "Error",
"code": 2707
},
"props.children are specified twice. The attribute named 'children' will be overwritten.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

name for children should be variable, and i would not include props here.

@yuit
Copy link
Contributor Author

yuit commented Apr 17, 2017

ping @RyanCavanaugh @mhegazy

@@ -423,6 +423,9 @@ namespace ts {

let _jsxNamespace: string;
let _jsxFactoryEntity: EntityName;
let _jsxElementAttribPropInterfaceSymbol: Symbol; // JSX.ElementAttributesProperty [symbol]
Copy link
Member

Choose a reason for hiding this comment

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

A comment would be good here

Copy link
Member

Choose a reason for hiding this comment

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

(A longer one explaining it, that is)

// interface ElementAttributesProperty {
// props: {
// children?: any;
// };
Copy link
Member

Choose a reason for hiding this comment

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

Let's figure out some other way to do this - this is too opaque and might not combine well with future work

error(attribsPropTypeSym.declarations[0], Diagnostics.The_global_type_JSX_0_may_not_have_more_than_one_property, JsxNames.ElementAttributesPropertyNameContainer);
return undefined;
// No interface exists, so the element attributes type will be an implicit any
_jsxElementPropertiesName = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean we'll keep doing this logic repeatedly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep if we can't find the JSXElement. May be it is better to be empty string to indicate that we already done this

@@ -14574,7 +14672,7 @@ namespace ts {
// We can figure that out by resolving attributes property and check number of properties in the resolved type
// If the call has correct arity, we will then check if the argument type and parameter type is assignable

const callIsIncomplete = node.attributes.end === node.end; // If we are missing the close "/>", the call is incomplete
const callIsIncomplete = node.attributes.end === node.end; // If we are missing the close "/>", the call is incoplete
Copy link
Member

Choose a reason for hiding this comment

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

Undo spelling regression

@@ -366,7 +366,7 @@ namespace ts {
return computeLineAndCharacterOfPosition(getLineStarts(sourceFile), position);
}

export function isWhiteSpace(ch: number): boolean {
export function isWhiteSpaceLike(ch: number): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Why the rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is not just WhiteSpace but Whitespace and empty line.

@RyanCavanaugh
Copy link
Member

GTG after lint fixes

@yuit
Copy link
Contributor Author

yuit commented Apr 21, 2017

Thanks @RyanCavanaugh for reviewing 🍰 🍰

@yuit yuit merged commit a1a2006 into master Apr 21, 2017
@yuit yuit deleted the master-jsxChildren branch April 21, 2017 17:02
@donaldpipowitch
Copy link
Contributor

Thank you! This is a great feature.

@RyanCavanaugh
Copy link
Member

@yuit can you write up some minimal docs? I can fill in some details but would like you to write down the basics

@radix
Copy link

radix commented Jun 10, 2017

The issue that this is meant to resolve has examples that restrict the type of the components in children, but I don't see an example in this PR that does that. Is it actually possible?

For example, in the linked issue there is a a use-case where a Modal component only allows ModalHeader, ModalBody, or ModalFooter components as children. Is that actually possible with this change?

@DanielRosenwasser
Copy link
Member

I see you've also been checking on SO: https://stackoverflow.com/questions/44475309/how-do-i-restrict-the-type-of-react-children-in-typescript-using-the-newly-adde

I think that the issue here is that all you really get back from a rendered component is a JSX Element. There's not really any information in thre to specify where the element came from. I'm not sure if @yuit or @RyanCavanaugh have had any thoughts on this.

@radix
Copy link

radix commented Jun 11, 2017

yeah, I have become skeptical that what I'm trying to do is possible, which makes me think that #13618 should not have been closed, since it is specifically about getting type restrictions on what kind of components are used as children of a react component. But I could very well be missing something!

@donaldpipowitch
Copy link
Contributor

donaldpipowitch commented Jun 12, 2017

I think that the issue here is that all you really get back from a rendered component is a JSX Element. There's not really any information in thre to specify where the element came from.

Yeah, this is what I saw in my tests, too. You can't say something like this:

import React, { Component } from 'react';
import { render } from 'react-dom';

class Foo extends Component<{}, void> {
  render() {
    return <p>Hello from Foo!</p>;
  }
}

interface BarProps {
  children?: Foo | Array<Foo>;
}

const Bar = ({ children }: BarProps) => <div>This must be Foo(s): {children}.</div>;

render(
  <div>
    <Bar>
      <Foo /> {/* invalid - is treated as `JSX.Element`, not `Foo` */}
    </Bar>
  </div>,
  document.getElementById('app')
);

@radix
Copy link

radix commented Jun 12, 2017

If this really can't be done, then I suggest re-opening #13618, since it's specifically about typing the components used as children (should I comment as such there?)

@Hotell
Copy link

Hotell commented Jul 15, 2017

any "last words" on this folks ?

If this really can't be done, then I suggest re-opening #13618, since it's specifically about typing the components used as children (should I comment as such there?)

cc @yuit @DanielRosenwasser @RyanCavanaugh thx!

@zackify
Copy link

zackify commented Oct 2, 2017

Would love to see support for typing children! 👍

@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants