acornjs / acorn-jsx

Alternative, faster React.js JSX parser
MIT License
646 stars 72 forks source link

Should this parse? #27

Closed nzakas closed 8 years ago

nzakas commented 8 years ago

In Espree, we have this test case throwing an error (came from Esprima-FB originally):

<a.b:c></a.b:c>;

In Acorn-JSX, this parses okay. I'm assuming Acorn-JSX is correct, but just want to confirm that this is the correct behavior.

sebmck commented 8 years ago

It doesn't look like the JSX grammar allows this. cc @sebmarkbage just to get absolute confirmation.

sebmarkbage commented 8 years ago

It should not, no. The rationale is, it's either XML or it is a JS expression. Not both or a third thing.

sebmck commented 8 years ago

Thanks for the confirmation @sebmarkbage!

nzakas commented 8 years ago

I'm a bit confused - if this code shouldn't parse, then there's a bug. Shouldn't this issue be open?

RReverser commented 8 years ago

Oh sorry, I thought it was confirmed that current behavior is desired.

I remember asking @sebmarkbage about why : syntax is needed at all if it's not used, and he told that it might be used by various custom implementations - e.g. FB uses it for localization. So I decided that there might be custom implementations that would benefit from wider support as well.

nzakas commented 8 years ago

If I'm reading this correctly, : is fine, it's the a.b that's the problem.

RReverser commented 8 years ago

That's why I said "wider support". If : is used only by some custom transformers, why not give them opportunity to have MemberExpression in this prefix as well?

nzakas commented 8 years ago

Oh I see what you mean. I think the argument against wider support would be creating code that couldn't be used in all locations?

The tricky part here is that a.b is the namespace and c is the element name. According to the spec, this is perfectly valid:

<a:b.c></a:b.c>

Dots are allowed in element names but not in namespace names.

AFAIK, dots are allowed in XML namespaces, so the JSX spec deviates from that in this regard.

I don't really have a strong opinion either way, I just want to make sure what I'm doing is consistent with expectations.

RReverser commented 8 years ago

AFAIK, dots are allowed in XML namespaces, so the JSX spec deviates from that in this regard.

Same thoughts. If some transformer wants to use namespaces at all, why not give wider yet XML-compliant support for namespaces?

I don't have strong opinion neither, but I don't see any reasons for reducing this to just identifier.

sebmarkbage commented 8 years ago

<a:b.c></a:b.c> is not allowed according to the spec. JSXElementName can either be JSXNamedspacedName or a JSXMemberExpression not both.

RReverser commented 8 years ago

@sebmarkbage But why so? Regarding e.g. mentioned localization transformer - why <en:Component /> is valid but <en:Namespace.Component /> is not?

sebmarkbage commented 8 years ago

It doesn't capture a locally scoped identifier. A namespace is more like the dash. Some arbitrary string.

There's not really any reason to forbid it other than for the contextual resolution disambiguation.

E.g. is treated as a local variable in React where as or would be a string. That's kind of lame though.

Are there other missing characters that are not part of JS identifiers?

RReverser commented 8 years ago

It doesn't capture a locally scoped identifier. A namespace is more like the dash. Some arbitrary string.

That feels kinda inconsistent with regular components. I (as a user) prefer names of any components (whether XML-namespaced or not) to have the same rules for namings.

sebmarkbage commented 8 years ago

What would it mean to have an XML namespaced local binding?

RReverser commented 8 years ago

Depends on transformer. I was considering namespace part purely as additional "marker" for transformers that might use it's left part for custom stuff (e.g. name of language or class name for transformation etc.) and right part as regular component name.

RReverser commented 8 years ago

@nzakas @teppeis If this would be an opt-in and throw by default, would be that sufficient for your needs?

nzakas commented 8 years ago

Yup, that would be great.

teppeis commented 8 years ago

@RReverser excellent!

RReverser commented 8 years ago

Published 3.0 with support for two new options:

nzakas commented 8 years ago

Awesome!