Attribute spec fixes. #10
Conversation
RReverser
commented
Sep 2, 2014
RReverser
commented
- JSXAttributeValue can be JSXElement (supported both in Esprima-FB and Acorn-JSX and reflected in tests).
- JSXSpreadAttribute is "subclass" of JSXAttribute. That also simplified JSXAttributes (list) rule.
- JSXAttributeValue can be JSXElement (supported both in Esprima-FB and Acorn-JSX and reflected in tests). - JSXSpreadAttribute is "subclass" of JSXAttribute. That also simplified JSXAttributes (list) rule.
|
Just stating my opinion, I'm not a fan of:
It's really cluttered and I doubt it's common enough to warrant the special treatment, If we do keep it, I feel like the same courtesy should be extended to numbers |
|
@syranide We can like it or not, but this is already used by some projects built on React, and I think that spec should reflect all of existing working features so React projects could conform to this spec. |
|
@syranide @RReverser Actually, the React jstransform pass has never supported the brace-less syntax for attributes. |
|
@sebmarkbage What do you think? |
|
I think we should support JSXElements as attribute values. It's cool and I don't see any syntactical reason why we couldn't support it (famous last words). A transpiler can opt-out. It encourages use of properties instead of children which is helpful for React keys so it could be good for React too. I'm not sure about making JSXSpreadAttribute a subclass of JSXAttribute though. It's nice to be able to refer to them individually. Especially since JSXAttribute is so much more common. With this change, there's no terminology to refer to a name/value pair. Note that the ES6 spec also does not group these together as one thing. |
Well, ES6 doesn't support any kind of attributes or even spread properties in object literals, so I'm not sure it's correct to compare those.
As well as we don't have terminology for not-self-closed-element. I think these rules are rather for lexer (syntax only) and not for defining terminology - this task should be addressed by exposing parser AST specification (#14) which contains strict inheritance, node types and properties of each parsed node. |
|
IMO, the Mozilla AST specification does a poor job introducing terminology because it conflates unrelated nodes (e.g. SpreadElement in esprima and properties with the "kind" tag). Often in the interest of backwards compatibility. I never hear people refer to as Mozilla's AST as authoritative when talking about terminology. It's always a reference to the ES6 spec. Maybe it's just my little world, but I do think that an AST specification shouldn't be the authoritative place for language terminology. It's rare that you would only know how to convert and handle not-self-closed-elements and not self-closed-elements. It's very common to not know how to deal with spread in a transform. So you should be able to narrow that down and talk about attributes alone. Besides, the unique name needs to exist in some form. What are you planning to call the XJSAttribute in the AST? Why can't it be named in this spec too? |
Agree, but I don't see why do we need that separate
It was invented before me - in Esprima-FB it's just XJSAttribute and XJSSpreadAttribute. Only for semantic describing purposes, I thought moving them under one parent is a good idea since 1) well, they are both still XJSAttributes so it becomes clear in one place what attribute options we have and 2) XJSAttributes list rule becomes a bit simpler, too. |
|
I separated JSXSelfClosingElement only for readability. No other reason, really. |
|
Isn't it confusing that JSXAttribute in the syntax spec is referring to both key/value and spread but in the AST spec it's referring to only key/value? Seems like it would be confusing which terminology is the right one. |
|
@sebmarkbage Even now they are not same, so even if it's confusing, then it's not smth brought by this PR. Current differences:
|
|
So I think those are not conflicting, but should be treater as different rules and so representations for tokenizer and structured parser. |
|
One more wrong doesn't make a right. :) I guess I don't understand why this change is better/simpler when it adds some confusion. It's prettier but comes with the risk of confusing terminology, worse than it already is. |
|
@sebmarkbage Ok, I see your point. So what solution would you prefer? On one hand, we could save this "prettyness" by just adding JSXNamedAttribute to avoid confusion, on other - I can revert that part completely if you wish. |
|
Maybe just revert it for now so that I can pull in the JSXElement part. We can continue the JSXAttribute discussion in another pull-request. JSXNamedAttribute could work but then the AST should be updated too. One thing to consider is that "spread" is conceptually like "expanding" extra JSXNamedAttributes. |
|
@sebmarkbage Something weird just happened. I tried to revert+rebase and my fork lost link to it's origin (facebook/jsx). Have never seen this with Github earlier. |
|
Oh, it's probably because you had the ssh link. I think we set up the team org differently so you may not have push access anymore. Just use the https or git link instead. |
|
@sebmarkbage But now it allowed me to create second fork. This is so weird. Anyway, here it is: #15 |
|
@sebmarkbage Ah, may be. |
|
Closing this in favor of #15 then. |
|
So when #14 is merged, I'll remove old fork and will continue work on new one. |
|
@RReverser When a private repo is made public, its forks get detached. |
|
@spicyj Thanks for clarifying. |
