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

Add JSXSpreadChild #59

Merged
merged 1 commit into from
Jul 2, 2016
Merged

Conversation

calebmer
Copy link
Contributor

As per discussions in #57 and babel/babylon#42.

This syntax is the same one I used in writing babel/babylon#42

cc @syranide, @sebmarkbage, @kittens

@calebmer
Copy link
Contributor Author

calebmer commented Jul 2, 2016

Are there any comments? Could we merge this? I’d like to see this functionality adopted in parsers, so it’s important to see this extension in the language.

@sebmarkbage sebmarkbage merged commit 94bc381 into facebook:master Jul 2, 2016
@sebmarkbage
Copy link
Contributor

Seems fine to me. Merged.

@calebmer calebmer deleted the feat/spread-children branch July 2, 2016 21:45
@@ -138,7 +138,7 @@ JSXChild :

- JSXText
- JSXElement
- `{` AssignmentExpression<sub>opt</sub> `}`
- `{` JSXChildExpression `}`
Copy link
Contributor

@RReverser RReverser Jul 6, 2016

Choose a reason for hiding this comment

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

nit: I would put opt here. as it was before Technically it doesn't matter for the grammar, but would be cleaner to see that the content between braces is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like that better too 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #61

@RReverser
Copy link
Contributor

@calebmer @sebmarkbage Does anyone remember why we introduced a separate node type for this and not reused SpreadElement from ESTree which has the same structure and visual syntax?

@calebmer
Copy link
Contributor Author

calebmer commented Jun 2, 2017

@RReverser I don’t think there was a specific reason. I defaulted to creating a new type which may be better for extensibility in the future.

@sebmarkbage
Copy link
Contributor

What does ESTree do for spread properties (object spread)?

@RReverser
Copy link
Contributor

@sebmarkbage

interface SpreadElement <: Node {
    type: "SpreadElement";
    argument: Expression;
}

@RReverser
Copy link
Contributor

@sebmarkbage Ah sorry, just realised that I misunderstood your question. See https://github.com/estree/estree/blob/master/experimental/rest-spread-properties.md - it also reuses SpreadElement / RestElement, hence my suggestion to use the same one in JSX for uniformity.

@sebmarkbage
Copy link
Contributor

I'm skeptical of seeking reuse for things that are specified as separate things in the syntax spec. It seems like a fragile future compatibility issue and convenience issue. E.g. for matchers in Babel or for implementing evaluators.

According to the last TC39 meeting, rest properties will no longer allow a nested pattern - unlike rest element. Only identifiers. So they've already diverged and might diverge more in the future.

@RReverser
Copy link
Contributor

shrugs AST is mostly for syntactic representation of things, there are better IRs for evaluators and checkers. And syntactically this is indeed ... + expression in both places, so can be used by parsers and serializers as such.

facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Nov 9, 2017
Summary: In facebook/jsx#59 spread children were added to the JSX specification. This adds type checking and parsing for spread children for CSX.

Reviewed By: calebmer

Differential Revision: D6236379

fbshipit-source-id: 5eca3b7b3748a47ff25e257fcb3c4778a73a5495
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

3 participants