Elaborate jsx children elementwise #29264

weswigham · 2019-01-04T19:13:21Z

This really exposed how wonky our JSX children checking is. The fact that a single-element-child isn't assignable to an array target (because it's not inserted into an array) is real strange.

Anyways, @DanielRosenwasser 's examples also exposed some problems with how we calculated contextual types for JSX children, which are also fixed in this PR.

DanielRosenwasser · 2019-01-08T19:01:42Z

tests/baselines/reference/checkJsxChildrenProperty7.errors.txt

-!!! error TS2322:         Type 'string | Element' is not assignable to type 'Element'.
-!!! error TS2322:           Type 'string' is not assignable to type 'Element'.
+                                           ~~
+!!! error TS2322: Type 'string' is not assignable to type 'Element'.


This is a little too subtle. Is it possible to make this

'{0}' components don't accept text as child elements. Text in JSX has the type 'string', but the expected type of '{1}' is '{2}'.

DanielRosenwasser · 2019-01-08T19:10:30Z

src/compiler/checker.ts

+                    return { errorNode: child, innerExpression: child.expression, nameType };
+                case SyntaxKind.JsxText:
+                    if (child.containsOnlyWhiteSpaces) {
+                        break; // Whitespace only jsx text isn't real jsx text


<Foo> </Foo>

will have a JsxText of '\n' but it'll get eliminated before it's converted to the JSX factory call. It won't become a call to React.createElement(Foo, {}, '\n'), but rather React.createElement(Foo).

It will be real text if it's placed between two other JSX elements and doesn't look like newline + indent (<><Foo/> <Bar/></> has 3 children), but I think this is only checking the single child case.

DanielRosenwasser · 2019-01-08T19:12:06Z

src/compiler/diagnosticMessages.json

+        "category": "Error",
+        "code": 2745
+    },
+    "Target JSX element expects a '{0}' prop of type '{1}', but multiple children were provided.": {


This JSX element expects its '{0}' prop to take a single child of type '{1}', but multiple children were provided.

DanielRosenwasser · 2019-01-08T19:14:45Z

src/compiler/diagnosticMessages.json

@@ -2541,6 +2541,14 @@
        "category": "Error",
        "code": 2744
    },
+    "Target JSX element expects a '{0}' prop of type '{1}', but only a single child was provided.": {


This JSX element's '{0}' prop expects type '{1}' which requires multiple children, but only a single child was provided.

Jessidhia · 2019-01-10T05:23:59Z

src/compiler/checker.ts

+                if (!length(validChildren)) {
+                    return result;
+                }
+                const moreThanOneRealChildren = length(validChildren) > 1;


This is hardcoding React-specific knowledge (preact makes no distinction between 1 or many children, it's always array), but it's still better than how it is right now.

This does mean that "The fact that a single-element-child isn't assignable to an array target (because it's not inserted into an array) is real strange." is actually a correct implementation of React at runtime.

ick, react-specific jsx checker code. This and the (now incorrect) sfc return type check. 🤕

This is why React tells you to always use React.Children when dealing with children, but nobody listens... 😥

React.Children provides utilities for dealing with the this.props.children opaque data structure.

opaque

OK.

Jessidhia · 2019-01-10T05:27:50Z

src/compiler/checker.ts

+                }
+                const moreThanOneRealChildren = length(validChildren) > 1;
+                const arrayLikeTargetParts = filterType(childrenTargetType, isArrayOrTupleLikeType);
+                const nonArrayLikeTargetParts = filterType(childrenTargetType, t => !isArrayOrTupleLikeType(t));


Do you think this fixes children not being caught in excess property checks? IIUC if there is no children prop these both should become neverType, though the error message will probably be confusing.

The react .d.ts unconditionally mixes children into class components, and so does React.FunctionComponent for those; so if you actually base your component type on the react builtin types, nope. However if you write your own unannotated function component, eg

const Tag = (x: {}) => <div></div>; // OK const k1 = <Tag />; const k2 = <Tag></Tag>; // Not OK (excess children) const k3 = <Tag children={<div></div>} />; const k4 = <Tag><div></div></Tag>; const k5 = <Tag><div></div><div></div></Tag>;

it seems to work OK.

That's not dependent on this PR, however - that's how it currently behaves. (This PR only adjusts error reporting mostly, after all)

Ick, works less OK when you have props in common:

const k4 = <Tag key="1"><div></div></Tag>; // should error but doesn't const k5 = <Tag key="1"><div></div><div></div></Tag>; // should error but doesn't

Hahahahahahaha the children prop thing is defs a bug on our part. It's excluded from excess property checks because it's missing a valueDeclaration (whose parent is the containing attributes).

Yep, this is that bug I found that time I did try to stop the unconditional mixing in of children :)

There is now a fix

weswigham · 2019-01-11T01:08:50Z

@DanielRosenwasser wanna look over the new messages?

DanielRosenwasser

Bellissimo

DanielRosenwasser · 2019-01-15T19:28:25Z

tests/baselines/reference/checkJsxChildrenProperty4.errors.txt




                { user => (
+                  ~~~~~~~~~


Would be awesome if we could make multi-line error spans less, uh, bad.

* origin/master: (64 commits) Fix resolution of properties from prototype assignment in JS (microsoft#29302) Include all flow nodes made within `try` blocks as antecedents for `catch` or `finally` blocks (microsoft#29466) Don't treat interfaces as implementations Make the relationship between partial mapped types and the empty object not apply for subtype relationship (microsoft#29384) Add missing arity check on second inference pass (microsoft#29386) renames add missing type annotation PR feedback Illustrate a case that isn't handled correctly Add fourslash tests Consider JSX namespace imports when moving statements between files Fix gulp builds not building some targets Update user baselines (microsoft#29444) Add opt-in user preference for prefix and suffix text on renames (microsoft#29314) Fake up value declaration for synthetic jsx children symbol so they get excess property checked (microsoft#29359) Add regression test. (microsoft#29433) Elaborate jsx children elementwise (microsoft#29264) Add regression test PR feedback Fix trailing whitespace ...

weswigham requested review from DanielRosenwasser and sandersn January 4, 2019 19:13

DanielRosenwasser requested changes Jan 8, 2019

View reviewed changes

Jessidhia reviewed Jan 10, 2019

View reviewed changes

weswigham added 4 commits January 10, 2019 16:13

Heavy WIP, but has good contextual typing fix

6b7a658

Add arity error, refine messages and spans

8192485

Small error message change

b9c9909

Better error messages, text-specific message

71b417b

weswigham force-pushed the elaborate-jsx-children-elementwise branch from 5dc1782 to 71b417b Compare January 11, 2019 00:46

weswigham mentioned this pull request Jan 11, 2019

Fake up value declaration for synthetic jsx children symbol so they get excess property checked #29359

Merged

DanielRosenwasser approved these changes Jan 15, 2019

View reviewed changes

weswigham merged commit 4968989 into microsoft:master Jan 15, 2019

weswigham deleted the elaborate-jsx-children-elementwise branch January 15, 2019 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elaborate jsx children elementwise #29264

Elaborate jsx children elementwise #29264

weswigham commented Jan 4, 2019

DanielRosenwasser Jan 8, 2019

DanielRosenwasser Jan 8, 2019

Jessidhia Jan 10, 2019 •

edited

Loading

DanielRosenwasser Jan 8, 2019

DanielRosenwasser Jan 8, 2019

Jessidhia Jan 10, 2019 •

edited

Loading

weswigham Jan 11, 2019

Jessidhia Jan 11, 2019

weswigham Jan 11, 2019

Jessidhia Jan 10, 2019

weswigham Jan 11, 2019

weswigham Jan 11, 2019

weswigham Jan 11, 2019

weswigham Jan 11, 2019

Jessidhia Jan 11, 2019 •

edited

Loading

weswigham Jan 11, 2019

weswigham commented Jan 11, 2019

DanielRosenwasser left a comment •

edited

Loading

DanielRosenwasser Jan 15, 2019

Elaborate jsx children elementwise #29264

Elaborate jsx children elementwise #29264

Conversation

weswigham commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Jan 11, 2019

DanielRosenwasser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Jan 10, 2019 •

edited

Loading

Jessidhia Jan 10, 2019 •

edited

Loading

Jessidhia Jan 11, 2019 •

edited

Loading

DanielRosenwasser left a comment •

edited

Loading