NotificationsYou must be signed in to change notification settings
Fork13.2k
Star107k

[Master] Type checking JSX children#15160

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

yuit merged 24 commits intomasterfrommaster-jsxChildren

Apr 21, 2017

Merged

[Master] Type checking JSX children#15160

yuit merged 24 commits intomasterfrommaster-jsxChildren

Apr 21, 2017

Conversation

Copy link

Contributor

yuit commentedApr 12, 2017•
edited
Loading

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 ischildren) 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 namedchildren. If there alreadychildren 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 intocheckExpression 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 orType JSX elements based on createElement function #14729.....

Examples

interfaceProp{a:number,b:string,children:string|JSX.Element}functionComp(p:Prop){return<div>{p.b}</div>;}// Error: missing childrenletk=<Compa={10}b="hi"/>;// Error: type not match// For this to work, "children: (string | JSX.Element)[]"letk1=<Compa={10}b="hi"><h2>Hello</h2>"hi"</Comp>// OKletk1=<Compa={10}b="hi"><h2>Hello</h2></Comp>;

interfaceIUser{Name:string;}interfaceIFetchUserProps{children:(user:IUser)=>JSX.Element;}classFetchUserextendsReact.Component<IFetchUserProps,any>{render(){returnthis.state            ?this.props.children(this.state.result)            :null;}}// ErrorfunctionUserName(){return(<FetchUser>{user=>(<h1>{user.NAme}</h1>//Thisisactuallyanerrornow;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)

Kanchalai Tanglertsampan added11 commits

April 7, 2017 16:41

Wip - type checking JSX children

b3846bf

Consider whitespace that won't be emitted to be different kind so tha…

f86a730

…t we won't include in typ-checking

Use JSX Attributes to contextually type children property

8e2dd38

Format checker

17417e9

Store scanning information whether JSXText is just an all whitespaces

4562fd0

wip-fixing consuming whitespace in children

c1ea303

Fixing consuming whitespace in children

4fa2312

Add tests

f099046

update baselines

eb0ad7f

Merge branch 'master-jsxChildren' ofhttps://github.com/Microsoft/Typ…

e5b95fc

…eScript into master-jsxChildren# Conflicts:#src/compiler/scanner.ts

Fix linting error

012f459

yuit requested a review fromRyanCavanaugh

April 12, 2017 20:11

msftclas added the cla-not-required label

Apr 12, 2017

Add tests and update baselines

58e2189

mhegazy reviewed

Apr 13, 2017

View reviewed changes

src/compiler/checker.ts Outdated

		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 withprops instead of hard coding it.

src/compiler/checker.ts Outdated

		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

src/compiler/diagnosticMessages.json Outdated

		"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 forchildren should be variable, and i would not includeprops here.

Kanchalai Tanglertsampan added8 commits

April 14, 2017 15:21

Remove a flag indicating the children is synthesized..We will give an…

bb8bab3

… error if you use children without specified such property

Read "children" property from react DTS file

c778a48

Update tests and baselines

dbf2a96

Update baselines and tests (2)

9a23b75

Fix linting error

7829cba

Update error message to use variable name for "children"

d290cf7

Merge branch 'master' into master-jsxChildren

f6f0813

# Conflicts:#src/compiler/checker.ts#src/compiler/diagnosticMessages.json

Fix baselines and linting error from merging with master

c13383a

Copy link

ContributorAuthor

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)

RyanCavanaugh reviewed

Apr 18, 2017

View reviewed changes

src/compiler/checker.ts Outdated

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

RyanCavanaugh reviewed

Apr 18, 2017

View reviewed changes

src/compiler/checker.ts Outdated

		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

ContributorAuthor

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

RyanCavanaugh reviewed

Apr 18, 2017

View reviewed changes

src/compiler/checker.ts Outdated

		// 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 isincomplete
		const callIsIncomplete = node.attributes.end === node.end; // If we are missing the close "/>", the call isincoplete

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

RyanCavanaugh reviewed

Apr 18, 2017

View reviewed changes

src/compiler/scanner.ts

		}

		exportfunctionisWhiteSpace(ch:number):boolean{
		exportfunctionisWhiteSpaceLike(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

ContributorAuthor

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.

Change how we look up children attribute from react.d.ts

e9cd3ad

Yui T added2 commits

April 19, 2017 22:33

Update tests and baselines

b7a30c1

Fix spelling and add comment

e03be45

Copy link

Member

RyanCavanaugh commentedApr 20, 2017

GTG after lint fixes

Fix linting

e7e13ec

Copy link

ContributorAuthor

yuit commentedApr 21, 2017

Thanks@RyanCavanaugh for reviewing 🍰 🍰

yuit merged commita1a2006 intomaster

Apr 21, 2017

yuit deleted the master-jsxChildren branch

April 21, 2017 17:02

aaronjensen mentioned this pull request

Apr 21, 2017

propertychildren not found in props of React element ...facebook/flow#1964

Closed

Copy link

Contributor

donaldpipowitch commentedApr 21, 2017

Thank you! This is a great feature.

Copy link

Member

RyanCavanaugh commentedApr 24, 2017

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

lostfictions mentioned this pull request

Apr 29, 2017

Move TypeScript defs to own folder and add testsreact-grid-layout/react-draggable#245

Merged

firewave-remo mentioned this pull request

May 4, 2017

Error on type checking JSX children#15583

Closed

yuit mentioned this pull request

May 4, 2017

[React] Include ElementChildrenAttribute for 2.3DefinitelyTyped/DefinitelyTyped#16327

Merged

15 tasks

Copy link

radix commentedJun 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 aModal component only allowsModalHeader,ModalBody, orModalFooter components as children. Is that actually possible with this change?

Copy link

Member

DanielRosenwasser commentedJun 11, 2017

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.

Copy link

radix commentedJun 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!

Copy link

Contributor

donaldpipowitch commentedJun 12, 2017•
edited
Loading

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 inmy tests, too. You can't say something like this:

importReact,{Component}from'react';import{render}from'react-dom';classFooextendsComponent<{},void>{render(){return<p>Hello from Foo!</p>;}}interfaceBarProps{children?:Foo|Array<Foo>;}constBar=({ 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'));

Copy link

radix commentedJun 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?)

donaldpipowitch mentioned this pull request

Jun 12, 2017

Suggestion: type checking for JSX children#13618

Closed

Copy link

Hotell commentedJul 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!

Copy link

zackify commentedOct 2, 2017

Would love to see support for typing children! 👍

microsoft locked and limited conversation to collaborators

Jun 21, 2018

Labels

None yet


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

Movatterモバイル変換

[Master] Type checking JSX children#15160

[Master] Type checking JSX children#15160

Uh oh!

Conversation

yuit commentedApr 12, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit commentedApr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh commentedApr 20, 2017

Uh oh!

yuit commentedApr 21, 2017

Uh oh!

donaldpipowitch commentedApr 21, 2017

Uh oh!

RyanCavanaugh commentedApr 24, 2017

Uh oh!

radix commentedJun 10, 2017

Uh oh!

DanielRosenwasser commentedJun 11, 2017

Uh oh!

radix commentedJun 11, 2017

Uh oh!

donaldpipowitch commentedJun 12, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

radix commentedJun 12, 2017

Uh oh!

Hotell commentedJul 15, 2017

Uh oh!

zackify commentedOct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

yuit commentedApr 12, 2017•
edited
Loading

donaldpipowitch commentedJun 12, 2017•
edited
Loading