Skip to content
This repository was archived by the owner on May 19, 2018. It is now read-only.

Add JSX spread children - #42

Merged
sebmck merged 1 commit into
babel:masterfrom
calebmer:feat/jsx-spread-children
Jul 13, 2016
Merged

Add JSX spread children#42
sebmck merged 1 commit into
babel:masterfrom
calebmer:feat/jsx-spread-children

Conversation

@calebmer

@calebmer calebmer commented Jun 19, 2016

Copy link
Copy Markdown
Contributor

This PR comes from a conversation in react/jsx#57 where I recommended adding the ability to spread children. For the full rationale see that issue, here’s an abridged version:

JSX with React allows us to do this:

const TodoList = ({ todos }) => (
  <div>
    {todos.map(todo => <Todo key={todo.id} todo={todo}/>)}
  </div>
)

Which translates to this:

const TodoList = ({ todos }) => ({
  type: 'div',
  children: [
    todos.map(todo => …)
  ],
})

That doesn’t make sense and it has disadvantages for simpler JSX consumers as well as type systems. Instead JSX should allow us to do this:

const TodoList = ({ todos }) => (
  <div>
    {...todos.map(todo => <Todo key={todo.id} todo={todo}/>)}
  </div>
)

Which would translate to this:

const TodoList = ({ todos }) => ({
  type: 'div',
  children: [
    ...todos.map(todo => …)
  ],
})

This PR aims to extend JSX by allowing the spreading of children like so:

<div>{...children}</div>

As another note, it seems like React itself will consider this an anti-pattern. However as JSX is an open standard used by more than just React we should consider the entire ecosystem when considering the merits of this extension. Let’s keep discussion of whether or not the extension should be added to JSX in react/jsx#57 and let’s talk about technical implementations here.

In react/jsx#57 the language extension got a +1 from @syranide (react/jsx#57 (comment)) and @sebmarkbage (react/jsx#57 (comment)).

@calebmer

calebmer commented Jun 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I’m not sure why jsx/basic/10 is failing. It looks like the comment blocks array at leadingComments/closingElement/expression/0/body/program has two identical copies the following comment block instead of the one tested for:

{
  "type": "CommentBlock",
  "value": " this is a comment ",
  "start": 4,
  "end": 27,
  "loc": {
    "start": {
      "line": 1,
      "column": 4
    },
    "end": {
      "line": 1,
      "column": 27
    }
  }
}

I’m pretty sure it has to do with the lookahead, but I couldn’t find any documentation on the matter.

Any ideas?

@sebmck

sebmck commented Jun 22, 2016

Copy link
Copy Markdown
Contributor

We'll have to hold off on this until it's been specced and added to the react/jsx#57 repo. TypeScript and Flow also support JSX so there's value in having this specced there so they can come into compliance as we add support.

@calebmer calebmer mentioned this pull request Jun 22, 2016
Closed
@sebmarkbage

sebmarkbage commented Jun 23, 2016

Copy link
Copy Markdown

I don't think either TypeScript or Flow support non-React variants of JSX atm so it probably doesn't matter but we can add it. We also don't support namespaces which is speced.

@calebmer calebmer mentioned this pull request Jun 25, 2016
Merged
@calebmer

calebmer commented Jul 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@kittens: The language extension this PR implements was just added to the JSX specification (react/jsx#59). What’s remaining to get this PR merged, and can you comment on the failing test?

See my comment above for more details on from the reasons I found that might make it fail.

@calebmer

calebmer commented Jul 12, 2016

Copy link
Copy Markdown
Contributor Author

Just curious @kittens, what’s the status on this?

danez
danez reviewed Jul 12, 2016
View reviewed changes
Comment thread src/plugins/jsx/index.js
this.next();
node.expression = this.parseExpression();
this.expect(tt.braceR);
children.push(this.finishNode(node, "JSXSpreadChild"));

@danez danez Jul 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

JSXSpreadChild would need to be added to babel-types as well and also to babel-generator

@calebmer calebmer Jul 13, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@danez 👍 babel/babel#3575

@calebmer calebmer mentioned this pull request Jul 13, 2016
Closed
@sebmck
sebmck merged commit 3fad8cc into babel:master Jul 13, 2016
@sebmck

sebmck commented Jul 13, 2016

Copy link
Copy Markdown
Contributor

Thanks! We can follow up in the Babel PR.

@danez danez mentioned this pull request Jul 13, 2016
Merged
@calebmer
calebmer deleted the feat/jsx-spread-children branch July 13, 2016 09:32
@divmain divmain mentioned this pull request Jul 17, 2016
Merged
@jasonLaster jasonLaster mentioned this pull request Aug 12, 2017
Merged
@thejimmylin thejimmylin mentioned this pull request May 10, 2022
Open
@xiaochengzi6 xiaochengzi6 mentioned this pull request May 20, 2022
Open
@coderabbitai coderabbitai Bot mentioned this pull request Jun 29, 2026
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@calebmer @sebmck @sebmarkbage @danez
Sponsor
SponsoredKunjungi sekarang
Promo