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

Fix react-jsx spread children invalid emit #46565

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Oct 28, 2021

Since #45693 react-jsx transforms:

<div>{...a}{...b}</div> ->

_jsxs("div", { children: [...a, ...b] }, void 0); // Good (HEAD)

vs.

_jsxs("div", { children: [a, b] }, void 0); // Pre-#45693 behavior

but a single spread child <div>{...a}</div> ->

_jsx("div", { children: ...a }, void 0); // Syntax error (HEAD)

The classic react transform isn't affected (handles a single spread child), FYI: <div>{...a}</div> ->

React.createElement("div", null, ...a); // Good (HEAD)

This PR fixes the react-jsx transform in case of a single spread child: <div>{...a}</div> ->

_jsxs("div", { children: [...a] }, void 0); // Good (this PR)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 28, 2021
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but just out of curiosity, why do we even allow this? I tried this in the Babel playground and they just error with

Spread children are not supported in React.

🤷

@andrewbranch
Copy link
Member

why do we even allow this

I specifically mean since the JSX emit mode has “react” in its name—should we also be emitting a grammar error in that mode?

@jablko
Copy link
Contributor Author

jablko commented Oct 28, 2021

The jsxFactory/jsxImportSource options allow for other runtimes, e.g. ones that distinguish between <div>{...todos}</div> and <div>{todos}</div>: facebook/jsx#57 (comment)

If those options are unset/set to React.createElement/react then it'd be fine to emit an error, I think 👍

@andrewbranch
Copy link
Member

Not a priority right now since this syntax has been allowed but broken for some time, but maybe something we should think about next cycle.

@andrewbranch
Copy link
Member

@sandersn or @weswigham can one of you double check this real quick, whoever gets here first? Hit merge if it looks good to you. Thanks!

tagName,
objectProperties,
keyAttr,
(nonWhitespaceChildren[0] as JsxExpression)?.dotDotDotToken ? 2 : length(nonWhitespaceChildren),
Copy link
Member

Choose a reason for hiding this comment

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

sigh if we're gunna manipulate the number like this, we should change the parameter to a isStaticChildren boolean flag instead. Don't want someone revisiting this in the future and question why it's sometimes not actually the length of the children.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

@jablko jablko force-pushed the patch-43 branch 2 times, most recently from 7dcfc96 to 6f38b96 Compare October 28, 2021 22:30
@weswigham weswigham merged commit 9b1ba8f into microsoft:main Oct 29, 2021
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
* Fix react-jsx spread children invalid emit

* Update Baselines and/or Applied Lint Fixes

* Change childrenLength parameter -> isStaticChildren

Co-authored-by: TypeScript Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants