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

Feat: add jsx spread children support #7294

Merged
merged 2 commits into from Nov 28, 2023
Merged

Feat: add jsx spread children support #7294

merged 2 commits into from Nov 28, 2023

Conversation

rhyzx
Copy link
Contributor

@rhyzx rhyzx commented Nov 24, 2023

What does this PR do?

Example

const a = <h1>{...[123]}</h1>

Output

Before:
error: Unexpected ...
After:
var a = jsx_dev_runtime.jsxDEV("h1", {
  children: [...[123]]
}, undefined, true, undefined, this);

This feature is already supported by most transpilers, eg:
Typescript
Esbuild
SWC

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I am willing to add test code for this feature, but I need some help.
As I understand it, the test code should be placed in test/snippets, test/snapshots.
However, the scripts/browser.js that tests them does not seem to be able to execute properly.

@rhyzx rhyzx changed the title Add jsx spread children support Feat: add jsx spread children support Nov 24, 2023
Copy link
Collaborator

@paperdave paperdave left a comment

Choose a reason for hiding this comment

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

can you add a test to transpiler.test.ts?

@paperdave
Copy link
Collaborator

also, i wonder if we can detect an array literal spread, to emit [1, 2, 3] instead of [...[1, 2, 3]]

@rhyzx
Copy link
Contributor Author

rhyzx commented Nov 27, 2023

can you add a test to transpiler.test.ts?

DONE

@rhyzx
Copy link
Contributor Author

rhyzx commented Nov 27, 2023

also, i wonder if we can detect an array literal spread, to emit [1, 2, 3] instead of [...[1, 2, 3]]

Yes, the optimizer should do this, and actually Bun has already implemented this.

// in.js
console.log([...[1, 2, 3]]);
console.log(
  jsxDEV(
    "div",
    {
      children: [...[1, 2, 3]],
    },
    undefined,
    true,
    undefined,
    this,
  ),
);
console.log(<div>{...[1, 2, 3]}</div>);

// bun-debug build test.js --minify
// ...
console.log([1,2,3]);
console.log(jsxDEV("div",{children:[1,2,3]},void 0,!0,void 0,OJ));
console.log(VJ.jsxDEV("div",{children:[...[1,2,3]]},void 0,!0,void 0,this))})
// ...

However, strangely, the code transformed through JSX does not execute this optimization.🤔

If I have time, I will attempt to fix this issue next.

Copy link
Collaborator

@paperdave paperdave left a comment

Choose a reason for hiding this comment

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

looks good, i would like to get the unreachable removed.

src/js_parser.zig Show resolved Hide resolved
@paperdave
Copy link
Collaborator

these test cases would be nice to have but dont pass right now. i dont really know how hard adding this optimization in would be.

expect(bun.transformSync("export var foo = <div>{...[1,2,3]}</div>")).toBe(
  `export var foo = jsxDEV("div", {
  children: [1, 2, 3]
}, undefined, true, undefined, this);
`,
);
expect(bun.transformSync("export var foo = <div>a{...[1,2,3]}b</div>")).toBe(
  `export var foo = jsxDEV("div", {
  children: [
    "a",
    1,
    2,
    3,
    "b"
  ]
}, undefined, true, undefined, this);
`,
);

@rhyzx
Copy link
Contributor Author

rhyzx commented Nov 28, 2023

these test cases would be nice to have but dont pass right now. i dont really know how hard adding this optimization in would be.

expect(bun.transformSync("export var foo = <div>{...[1,2,3]}</div>")).toBe(
  `export var foo = jsxDEV("div", {
  children: [1, 2, 3]
}, undefined, true, undefined, this);
`,
);
expect(bun.transformSync("export var foo = <div>a{...[1,2,3]}b</div>")).toBe(
  `export var foo = jsxDEV("div", {
  children: [
    "a",
    1,
    2,
    3,
    "b"
  ]
}, undefined, true, undefined, this);
`,
);

I've observed that esbuild also doesn't optimize spread children in jsx, see

Implementing this optimization may not be as easy as it seems. I think we should open a new issue to discuss it.

@rhyzx rhyzx requested a review from paperdave November 28, 2023 02:43
@paperdave paperdave self-assigned this Nov 28, 2023
@paperdave
Copy link
Collaborator

alright. looks good otherwise. will merge after tests run.

@paperdave paperdave merged commit 713d727 into oven-sh:main Nov 28, 2023
17 of 26 checks passed
@paperdave
Copy link
Collaborator

thank you

ryoppippi pushed a commit to ryoppippi/bun that referenced this pull request Feb 1, 2024
* feat: jsx spread children

* test: spread children
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants