Skip to content

Add support for JSX fragment syntax #19249

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 31, 2017
Merged

Add support for JSX fragment syntax #19249

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
abb3f58
Add support for JSX fragment syntax
uniqueiniquity Oct 14, 2017
269d37a
Update tests
uniqueiniquity Oct 16, 2017
5f8a1b5
Add baselines
uniqueiniquity Oct 17, 2017
845f573
Update API baselines
uniqueiniquity Oct 17, 2017
7566529
Respond to CR
uniqueiniquity Oct 17, 2017
a7f9ec0
Respond to CR
uniqueiniquity Oct 18, 2017
fb05886
Respond to CR
uniqueiniquity Oct 24, 2017
961495b
Remove unnecessary type annotation
uniqueiniquity Oct 25, 2017
5dd1a47
Error when using fragment with jsxFactory
uniqueiniquity Oct 24, 2017
a83ec41
Added test for preventing fragment with jsxFactory
uniqueiniquity Oct 25, 2017
3ebb2e8
Merge branch 'master' into jsxFragment
uniqueiniquity Oct 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3296,6 +3296,9 @@ namespace ts {
case SyntaxKind.JsxOpeningElement:
case SyntaxKind.JsxText:
case SyntaxKind.JsxClosingElement:
case SyntaxKind.JsxFragment:
case SyntaxKind.JsxOpeningFragment:
case SyntaxKind.JsxClosingFragment:
case SyntaxKind.JsxAttribute:
case SyntaxKind.JsxAttributes:
case SyntaxKind.JsxSpreadAttribute:
Expand Down
36 changes: 28 additions & 8 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13467,9 +13467,15 @@ namespace 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) ?
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this type annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

node.parent.parent :
node.parent.openingElement.attributes; // node.parent is JsxElement
isJsxElement(node.parent) ?
node.parent.openingElement.attributes :
undefined; // node.parent is JsxFragment with no attributes

if (!jsxAttributes) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

return undefined; // don't check children of a fragment
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

}

// When we trying to resolve JsxOpeningLikeElement as a stateless function element, we will already give its attributes a contextual type
// which is a type of the parameter of the signature we are trying out.
Expand Down Expand Up @@ -14049,13 +14055,13 @@ namespace ts {
}

function checkJsxSelfClosingElement(node: JsxSelfClosingElement): Type {
checkJsxOpeningLikeElement(node);
checkJsxOpeningLikeElementOrOpeningFragment(node);
return getJsxGlobalElementType() || anyType;
}

function checkJsxElement(node: JsxElement): Type {
// Check attributes
checkJsxOpeningLikeElement(node.openingElement);
checkJsxOpeningLikeElementOrOpeningFragment(node.openingElement);

// Perform resolution on the closing tag so that rename/go to definition/etc work
if (isJsxIntrinsicIdentifier(node.closingElement.tagName)) {
Expand All @@ -14068,6 +14074,11 @@ namespace ts {
return getJsxGlobalElementType() || anyType;
}

function checkJsxFragment(node: JsxFragment): Type {
checkJsxOpeningLikeElementOrOpeningFragment(node.openingFragment);
return getJsxGlobalElementType() || anyType;
}

/**
* Returns true iff the JSX element name would be a valid JS identifier, ignoring restrictions about keywords not being identifiers
*/
Expand Down Expand Up @@ -14731,14 +14742,19 @@ namespace ts {
}
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I take it back

function checkJsxOpeningLikeElement(node: JsxOpeningLikeElement) {
checkGrammarJsxElement(node);
function checkJsxOpeningLikeElementOrOpeningFragment(node: JsxOpeningLikeElement | JsxOpeningFragment) {
const isNodeOpeningLikeElement = isJsxOpeningLikeElement(node);

if (isNodeOpeningLikeElement) {
checkGrammarJsxElement(<JsxOpeningLikeElement>node);
}
checkJsxPreconditions(node);
// The reactNamespace/jsxFactory's root symbol should be marked as 'used' so we don't incorrectly elide its import.
// And if there is no reactNamespace/jsxFactory's symbol in scope when targeting React emit, we should issue an error.
const reactRefErr = diagnostics && compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
const reactNamespace = getJsxNamespace();
const reactSym = resolveName(node.tagName, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace, /*isUse*/ true);
const reactLocation = isNodeOpeningLikeElement ? (<JsxOpeningLikeElement>node).tagName : node;
const reactSym = resolveName(reactLocation, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace, /*isUse*/ true);
if (reactSym) {
// Mark local symbol as referenced here because it might not have been marked
// if jsx emit was not react as there wont be error being emitted
Expand All @@ -14750,7 +14766,9 @@ namespace ts {
}
}

checkJsxAttributesAssignableToTagNameAttributes(node);
if (isNodeOpeningLikeElement) {
checkJsxAttributesAssignableToTagNameAttributes(<JsxOpeningLikeElement>node);
}
}

/**
Expand Down Expand Up @@ -18518,6 +18536,8 @@ namespace ts {
return checkJsxElement(<JsxElement>node);
case SyntaxKind.JsxSelfClosingElement:
return checkJsxSelfClosingElement(<JsxSelfClosingElement>node);
case SyntaxKind.JsxFragment:
return checkJsxFragment(<JsxFragment>node);
case SyntaxKind.JsxAttributes:
return checkJsxAttributes(<JsxAttributes>node, checkMode);
case SyntaxKind.JsxOpeningElement:
Expand Down
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3607,6 +3607,14 @@
"category": "Error",
"code": 17013
},
"JSX fragment has no corresponding closing tag.": {
"category": "Error",
"code": 17014
},
"Expected corresponding closing tag for JSX fragment.": {
"category": "Error",
"code": 17015
},

"Circularity detected while resolving configuration: {0}": {
"category": "Error",
Expand Down
46 changes: 28 additions & 18 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,9 +699,11 @@ namespace ts {
case SyntaxKind.JsxText:
return emitJsxText(<JsxText>node);
case SyntaxKind.JsxOpeningElement:
return emitJsxOpeningElement(<JsxOpeningElement>node);
case SyntaxKind.JsxOpeningFragment:
return emitJsxOpeningElementOrFragment(<JsxOpeningElement>node);
case SyntaxKind.JsxClosingElement:
return emitJsxClosingElement(<JsxClosingElement>node);
case SyntaxKind.JsxClosingFragment:
return emitJsxClosingElementOrFragment(<JsxClosingElement>node);
case SyntaxKind.JsxAttribute:
return emitJsxAttribute(<JsxAttribute>node);
case SyntaxKind.JsxAttributes:
Expand Down Expand Up @@ -836,6 +838,8 @@ namespace ts {
return emitJsxElement(<JsxElement>node);
case SyntaxKind.JsxSelfClosingElement:
return emitJsxSelfClosingElement(<JsxSelfClosingElement>node);
case SyntaxKind.JsxFragment:
return emitJsxFragment(<JsxFragment>node);

// Transformation nodes
case SyntaxKind.PartiallyEmittedExpression:
Expand Down Expand Up @@ -2060,7 +2064,7 @@ namespace ts {

function emitJsxElement(node: JsxElement) {
emit(node.openingElement);
emitList(node, node.children, ListFormat.JsxElementChildren);
emitList(node, node.children, ListFormat.JsxElementOrFragmentChildren);
emit(node.closingElement);
}

Expand All @@ -2075,24 +2079,36 @@ namespace ts {
write("/>");
}

function emitJsxOpeningElement(node: JsxOpeningElement) {
function emitJsxFragment(node: JsxFragment) {
emit(node.openingFragment);
emitList(node, node.children, ListFormat.JsxElementOrFragmentChildren);
emit(node.closingFragment);
}

function emitJsxOpeningElementOrFragment(node: JsxOpeningElement | JsxOpeningFragment) {
write("<");
emitJsxTagName(node.tagName);
writeIfAny(node.attributes.properties, " ");
// We are checking here so we won't re-enter the emitting pipeline and emit extra sourcemap
if (node.attributes.properties && node.attributes.properties.length > 0) {
emit(node.attributes);

if (isJsxOpeningElement(node)) {
emitJsxTagName(node.tagName);
// We are checking here so we won't re-enter the emitting pipeline and emit extra sourcemap
if (node.attributes.properties && node.attributes.properties.length > 0) {
write(" ");
emit(node.attributes);
}
}

write(">");
}

function emitJsxText(node: JsxText) {
writer.writeLiteral(getTextOfNode(node, /*includeTrivia*/ true));
}

function emitJsxClosingElement(node: JsxClosingElement) {
function emitJsxClosingElementOrFragment(node: JsxClosingElement | JsxClosingFragment) {
write("</");
emitJsxTagName(node.tagName);
if (isJsxClosingElement(node)) {
emitJsxTagName(node.tagName);
}
write(">");
}

Expand Down Expand Up @@ -2611,12 +2627,6 @@ namespace ts {
writer.decreaseIndent();
}

function writeIfAny(nodes: NodeArray<Node>, text: string) {
if (some(nodes)) {
write(text);
}
}

function writeToken(token: SyntaxKind, pos: number, contextNode?: Node) {
return onEmitSourceMapOfToken
? onEmitSourceMapOfToken(contextNode, token, pos, writeTokenText)
Expand Down Expand Up @@ -3176,7 +3186,7 @@ namespace ts {
EnumMembers = CommaDelimited | Indented | MultiLine,
CaseBlockClauses = Indented | MultiLine,
NamedImportsOrExportsElements = CommaDelimited | SpaceBetweenSiblings | AllowTrailingComma | SingleLine | SpaceBetweenBraces,
JsxElementChildren = SingleLine | NoInterveningComments,
JsxElementOrFragmentChildren = SingleLine | NoInterveningComments,
JsxElementAttributes = SingleLine | SpaceBetweenSiblings | NoInterveningComments,
CaseOrDefaultClauseStatements = Indented | MultiLine | NoTrailingNewLine | OptionalIfEmpty,
HeritageClauseTypes = CommaDelimited | SpaceBetweenSiblings | SingleLine,
Expand Down
53 changes: 50 additions & 3 deletions src/compiler/factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2115,6 +2115,22 @@ namespace ts {
: node;
}

export function createJsxFragment(openingFragment: JsxOpeningFragment, children: ReadonlyArray<JsxChild>, closingFragment: JsxClosingFragment) {
const node = <JsxFragment>createSynthesizedNode(SyntaxKind.JsxFragment);
node.openingFragment = openingFragment;
node.children = createNodeArray(children);
node.closingFragment = closingFragment;
return node;
}

export function updateJsxFragment(node: JsxFragment, openingFragment: JsxOpeningFragment, children: ReadonlyArray<JsxChild>, closingFragment: JsxClosingFragment) {
return node.openingFragment !== openingFragment
|| node.children !== children
|| node.closingFragment !== closingFragment
? updateNode(createJsxFragment(openingFragment, children, closingFragment), node)
: node;
}

export function createJsxAttribute(name: Identifier, initializer: StringLiteral | JsxExpression) {
const node = <JsxAttribute>createSynthesizedNode(SyntaxKind.JsxAttribute);
node.name = name;
Expand Down Expand Up @@ -2951,7 +2967,7 @@ namespace ts {
);
}

function createReactNamespace(reactNamespace: string, parent: JsxOpeningLikeElement) {
function createReactNamespace(reactNamespace: string, parent: JsxOpeningLikeElement | JsxOpeningFragment) {
// To ensure the emit resolver can properly resolve the namespace, we need to
// treat this identifier as if it were a source tree node by clearing the `Synthesized`
// flag and setting a parent node.
Expand All @@ -2963,7 +2979,7 @@ namespace ts {
return react;
}

function createJsxFactoryExpressionFromEntityName(jsxFactory: EntityName, parent: JsxOpeningLikeElement): Expression {
function createJsxFactoryExpressionFromEntityName(jsxFactory: EntityName, parent: JsxOpeningLikeElement | JsxOpeningFragment): Expression {
if (isQualifiedName(jsxFactory)) {
const left = createJsxFactoryExpressionFromEntityName(jsxFactory.left, parent);
const right = createIdentifier(idText(jsxFactory.right));
Expand All @@ -2975,7 +2991,7 @@ namespace ts {
}
}

function createJsxFactoryExpression(jsxFactoryEntity: EntityName, reactNamespace: string, parent: JsxOpeningLikeElement): Expression {
function createJsxFactoryExpression(jsxFactoryEntity: EntityName, reactNamespace: string, parent: JsxOpeningLikeElement | JsxOpeningFragment): Expression {
return jsxFactoryEntity ?
createJsxFactoryExpressionFromEntityName(jsxFactoryEntity, parent) :
createPropertyAccess(
Expand Down Expand Up @@ -3016,6 +3032,37 @@ namespace ts {
);
}

export function createExpressionForJsxFragment(jsxFactoryEntity: EntityName, reactNamespace: string, children: Expression[], parentElement: JsxOpeningFragment, location: TextRange): LeftHandSideExpression {
const tagName = createPropertyAccess(
createReactNamespace(reactNamespace, parentElement),
"Fragment"
);
Copy link

@Jessidhia Jessidhia Oct 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So are you on board with issuing the error?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to issue an error.

Copy link

Choose a reason for hiding this comment

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

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


const argumentsList = [<Expression>tagName];
argumentsList.push(createNull());

if (children && children.length > 0) {
if (children.length > 1) {
for (const child of children) {
child.startsOnNewLine = true;
argumentsList.push(child);
}
}
else {
argumentsList.push(children[0]);
}
}

return setTextRange(
createCall(
createJsxFactoryExpression(jsxFactoryEntity, reactNamespace, parentElement),
/*typeArguments*/ undefined,
argumentsList
),
location
);
}

// Helpers

export function getHelperName(name: string) {
Expand Down
Loading