Add support for JSX fragment syntax by uniqueiniquity · Pull Request #19249 · microsoft/TypeScript

uniqueiniquity · 2017-10-17T16:52:51Z

Fixes #19094, which in turn reflects facebook/jsx#93.

This PR adds support for the <>...</> syntax for fragments in JSX. Under preserve emit, the syntax is maintained, and under react emit, fragment blocks are transformed into React.createElement(React.Fragment, null, ...) (and are thus affected by --jsxFactory). Due to similarities with the JSXElement syntax, the implementation shares much of the same code where appropriate.

RyanCavanaugh · 2017-10-17T17:30:27Z

src/compiler/checker.ts

-            // which is a type of the parameter of the signature we are trying out.
-            // If there is no contextual type (e.g. we are trying to resolve stateful component), get attributes type from resolving element's tagName
-            const attributesType = getContextualType(jsxAttributes);
+            if (jsxAttributes) {


Just do an early return of undefined here if jsxAttributes is undefined

RyanCavanaugh · 2017-10-17T17:31:35Z

src/compiler/checker.ts


-        function checkJsxOpeningLikeElement(node: JsxOpeningLikeElement) {
-            checkGrammarJsxElement(node);
+        function checkJsxOpeningLikeElementOrOpeningFragment(node: JsxOpeningLikeElement | JsxOpeningFragment) {


No need to rename this - a fragment is an element (IMHO?)

I take it back

RyanCavanaugh · 2017-10-17T17:38:16Z

src/compiler/parser.ts

+            else if (opening.kind === SyntaxKind.JsxOpeningFragment) {
+                const node = <JsxFragment>createNode(SyntaxKind.JsxFragment, opening.pos);
+                node.openingFragment = opening;
+


Nit extra newline

RyanCavanaugh · 2017-10-17T17:39:36Z

src/compiler/parser.ts

                    // If we hit EOF, issue the error at the tag that lacks the closing element
                    // rather than at the end of the file (which is useless)
-                    parseErrorAtPosition(openingTagName.pos, openingTagName.end - openingTagName.pos, Diagnostics.JSX_element_0_has_no_corresponding_closing_tag, getTextOfNodeFromSourceText(sourceText, openingTagName));
+                    if (isJsxOpeningElement(openingTag)) {


Reading this whole thing it seems like it'd be clearer if we wrote e.g. if (isJsxFragment(openingTag)) instead of isJsxOpeningElement since the former is more the thing we're special-casing

Restructured.

RyanCavanaugh · 2017-10-17T17:41:32Z

src/compiler/parser.ts

            return finishNode(node);
        }

+        function parseJsxClosingFragment(inExpressionContext: boolean): JsxClosingFragment {


Reading the spec, I don't see how we'd legally get into the inExpressionContext = false case since you can't write <foo><><div /></></foo>

The spec changed, now you can.

facebook/jsx#97

RyanCavanaugh · 2017-10-17T17:43:24Z

src/compiler/scanner.ts


+    /* @internal */
+    export function tokenIsIdentifierOrKeywordOrGreaterThan(token: SyntaxKind): boolean {
+        return token === SyntaxKind.GreaterThanToken || token >= SyntaxKind.Identifier;


Call tokenIsIdentifierOrKeyword in the right arm in case that logic changes

RyanCavanaugh · 2017-10-17T17:45:03Z

src/compiler/types.ts

    }

-    /// Either the opening tag in a <Tag>...</Tag> pair, or the lone <Tag /> in a self-closing form
+    /// Either the opening tag in a <Tag>...</Tag> pair, the opening tag in a <>...</> pair, or the lone <Tag /> in a self-closing form


This comment doesn't match our usage/terminology of isJsxOpeningLikeElement

RyanCavanaugh · 2017-10-17T17:52:05Z

tests/baselines/reference/checkJsxChildrenProperty14.errors.txt

+    }
+
+    // OK
+    let k1 = <Comp a={10} b="hi"><></><Button /><AnotherButton /></Comp>;


This should be a parse error

Or perhaps we should parse it leniently but issue a grammar error in checking

(disregard)

uniqueiniquity · 2017-10-17T23:46:13Z

@mhegazy could you take a look at this?

uniqueiniquity · 2017-10-18T17:00:18Z

Editor actions work as expected in VS and VS Code (i.e., go to def, find all references, etc are basically no-ops or return no results). I will be updating the syntax highlighter accordingly.

RyanCavanaugh · 2017-10-18T17:10:01Z

src/compiler/checker.ts

-                // If there is no contextual type (e.g. we are trying to resolve stateful component), get attributes type from resolving element's tagName
-                const attributesType = getContextualType(jsxAttributes);
+            if (!jsxAttributes) {
+                return anyType; // don't check children of a fragment


It's better to return undefined than anyType here because it will cause the calling code to bail out earlier

uniqueiniquity · 2017-10-18T17:23:54Z

Tracking syntax highlighter changes for VS Code at microsoft/TypeScript-TmLanguage#534.

Jessidhia · 2017-10-19T04:38:51Z

src/compiler/factory.ts

+        const tagName = createPropertyAccess(
+            createReactNamespace(reactNamespace, parentElement),
+            "Fragment"
+        );


Should this perhaps be customizable, similar to how --jsxFactory is? e.g. --jsxFragmentComponent?

Arguably not useful for anything but React and React clones, though; as other less similar frameworks would want to do other special processing on fragments.

I agree with your second point here; to take the example of Mithril, the framework supports fragments, but with a completely different approach than React will (i.e. m.fragment(attrs, children)) so it's not as convenient as simply replacing the call to createElement with --jsxFactory. It seems to make sense in this case to just preserve the JSX syntax as is for non-React libs and have each define their own transpilation depending on their implementation of fragments.

@Kovensky do you know what babel will do here? would it support another version of the paragma for the React.createElement?

checked with @hzoo over slack and does not seem that babel have plans for this in the immediate term.
I wounder if we should just make it an error to use Fragment + --jsx React + --jsxNamespace

@uniqueiniquity can you file an issue for adding the error.

Oh, I think I see now. I'm confusing jsxFactory and reactNamespace

So are you on board with issuing the error?

Probably, but feel free not to block this PR on it.

Updated the PR to issue an error.

In the Babel PR, I add a pragma for fragments, as @mhegazy suggested. https://github.com/babel/babel/pull/6552/files#diff-7e5b807d66ce513f501563ad8f8d03e6R117
https://github.com/babel/babel/pull/6552/files#diff-7e5b807d66ce513f501563ad8f8d03e6R89

Jessidhia · 2017-10-19T04:42:39Z

tests/baselines/reference/checkJsxChildrenProperty14.js

+}
+
+// OK
+let k4 = <SingleChildComp a={10} b="hi"><><Button /><AnotherButton /></></SingleChildComp>;


I'd kinda prefer for these to be a different type like, say, JSX.Fragment, but having one would not be useful anyway without nominal typing.

We'd like to make JSX.Element generic in the future - possibly looking it up and instantiating it based on the return type of the factory function; that would handle the Fragment case somewhat elegantly.

Generic JSX.Element<T> would be great. Outside of the React bubble, TypeScript-oriented developers like myself often wonder why the disconnect.

DanielRosenwasser · 2017-10-24T18:03:27Z

src/compiler/diagnosticMessages.json

+        "category": "Error",
+        "code": 17014
+    },
+    "Expected corresponding JSX fragment closing tag.": {


Expected corresponding closing tag for JSX fragment.

DanielRosenwasser · 2017-10-24T18:04:19Z

src/compiler/emitter.ts

+
+            if (isJsxOpeningElement(node)) {
+                emitJsxTagName(node.tagName);
+                writeIfAny(node.attributes.properties, " ");


You should move this into the below if check and replace the writeIfAny with a write

DanielRosenwasser · 2017-10-24T18:07:56Z

Looks reasonable apart from the nits I requested changes on. 👍

DanielRosenwasser · 2017-10-24T22:04:12Z

src/compiler/checker.ts

        function getContextualTypeForJsxExpression(node: JsxExpression): Type {
            // JSX expression can appear in two position : JSX Element's children or JSX attribute
-            const jsxAttributes = isJsxAttributeLike(node.parent) ?
+            const jsxAttributes: JsxAttributes = isJsxAttributeLike(node.parent) ?


Why do you need this type annotation?

uniqueiniquity added 4 commits October 17, 2017 09:38

Add support for JSX fragment syntax

abb3f58

Update tests

269d37a

Add baselines

5f8a1b5

Update API baselines

845f573

uniqueiniquity requested review from RyanCavanaugh and billti October 17, 2017 16:52

RyanCavanaugh reviewed Oct 17, 2017

View reviewed changes

Respond to CR

7566529

RyanCavanaugh reviewed Oct 18, 2017

View reviewed changes

RyanCavanaugh approved these changes Oct 18, 2017

View reviewed changes

Respond to CR

a7f9ec0

Jessidhia reviewed Oct 19, 2017

View reviewed changes

mhegazy approved these changes Oct 24, 2017

View reviewed changes

DanielRosenwasser requested changes Oct 24, 2017

View reviewed changes

Respond to CR

fb05886

DanielRosenwasser reviewed Oct 24, 2017

View reviewed changes

DanielRosenwasser approved these changes Oct 24, 2017

View reviewed changes

Remove unnecessary type annotation

961495b

duailibe mentioned this pull request Nov 27, 2017

Update TypeScript to 2.6.2 prettier/prettier#3331

Merged

This was referenced Nov 28, 2017

Update typescript in / from 2.6.1 to 2.6.2 mjohnson9/twitch-markov-bot#57

Open

Update typescript in /bundle from 2.4.2 to 2.6.2 src-zone/material#98

Closed

Update 35 dependencies from npm storybookjs/storybook#2429

Merged

pelotom mentioned this pull request Dec 6, 2017

jsx-wrap-multiline can't handle JSX fragment syntax palantir/tslint#3547

Closed

ghost mentioned this pull request Dec 19, 2017

Update typescript in / from 2.5.1 to 2.6.2 chauncey-garrett/dotfiles#63

Open

ghost mentioned this pull request Jan 22, 2018

Debug Failure. False expression: JsxText tokens should not be the first child of JsxElement | JsxSelfClosingElement in findRightmostChildNodeWithTokens during getQuickInfoAtPosition #20788

Closed

renovate-tests mentioned this pull request Feb 5, 2018

Update dependency typescript to v2.7.1 renovate-tests/slup#16

Open

manucorporat mentioned this pull request Feb 14, 2018

feat(render): support JSX fragment stenciljs/core#534

Closed

devuxer mentioned this pull request Mar 2, 2018

React.Fragment works but the short syntax (<>...</>) doesn't s-panferov/awesome-typescript-loader#542

Closed

dependabot-preview bot mentioned this pull request Mar 13, 2018

Bump typescript from 2.3.4 to 2.7.2 in /frontend Xenira/Shashki#64

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

uniqueiniquity commented Oct 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!

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 Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

uniqueiniquity commented Oct 17, 2017

Uh oh!

uniqueiniquity commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uniqueiniquity commented Oct 18, 2017

Uh oh!

Jessidhia Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

SMotaal Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

RyanCavanaugh Oct 17, 2017 •

edited

Loading

Jessidhia Oct 19, 2017 •

edited

Loading

SMotaal Nov 14, 2017 •

edited

Loading