This repository has been archived by the owner. It is now read-only.

Add JSX spread children #42

Merged
merged 1 commit into from Jul 13, 2016

Conversation

Projects
None yet
4 participants
@calebmer
Copy link
Contributor

calebmer commented Jun 19, 2016

This PR comes from a conversation in facebook/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 facebook/jsx#57 and let’s talk about technical implementations here.

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

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Jun 19, 2016

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?

@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Jun 22, 2016

We'll have to hold off on this until it's been specced and added to the facebook/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.

@sebmarkbage

This comment has been minimized.

Copy link

sebmarkbage commented Jun 23, 2016

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 referenced this pull request Jun 25, 2016

Merged

Add JSXSpreadChild #59

@calebmer

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Jul 2, 2016

@kittens: The language extension this PR implements was just added to the JSX specification (facebook/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

This comment has been minimized.

Copy link
Contributor Author

calebmer commented Jul 12, 2016

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

this.next();
node.expression = this.parseExpression();
this.expect(tt.braceR);
children.push(this.finishNode(node, "JSXSpreadChild"));

This comment has been minimized.

@danez

danez Jul 12, 2016

Member

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

This comment has been minimized.

@calebmer

calebmer Jul 13, 2016

Author Contributor

@kittens kittens merged commit 3fad8cc into babel:master Jul 13, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Jul 13, 2016

Thanks! We can follow up in the Babel PR.

@calebmer calebmer deleted the calebmer:feat/jsx-spread-children branch Jul 13, 2016

@divmain divmain referenced this pull request Jul 17, 2016

Merged

Comment node loc.filename #80

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.