Add support for XML/XJS identifier namespaces

[syranide]

Depends on approval of facebook/react#760

[syranide]

Updated as per our discussions, just shout if there's anything you want changed/improved.

[jeffmo]

I don't see why we need a special creator function...why not just use delegate.createMemberExpression() inside parseXJSMemberExpression? That way we don't have to invent a new node that is really just a copy of an existing node type...

[jeffmo]

Per convo in IRC, you're right that this makes the most sense...since it's property attribute will be of type XJSIdentifier (whereas regular MemberExpression nodes would have vanilla a Identifier)

[jeffmo]

Oh, and can you please add some tests to tests to test/fbtest.js?

[syranide]

@jeffmo Can add tests for sure, it seems like there should be some easy way to generate them? (and just manually check that they are indeed correct) Figured it out.

Also, I had overlooked checking the first parseXJSIdentifier for namespace, but this means the assert is present in two places. Would it perhaps be better to extend parseXJSIdentifier with a boolean indicating whether it should assert on namespaces?

Or perhaps better yet just do it properly, strip namespace from XJSIdentifier and instead introduce parseXJSNamespacedIdentifier which parses XJSIdentifier[:XJSIdentifier] and can return either createXJSIdentifier or createXJSNamespacedIdentifier? One possible downside/upside is that parseXJSMemberExpression will naturally fail when it parses a :, rather than after the fact.

[syranide]

Tests pushed.

[jeffmo]

I think wrapping identifiers in some kind of namespace node is the right thing to do longer term. If it didn't also mean that we'll have to make corresponding changes to the transform, I wouldn't be opposed to you taking a stab at it in this diff -- so let's just do that in a separate follow-up diff to simplify cross-compat between this change and the transform changes that'll need to happen.

As for adding a bool parameter to parseXJSIdentifier -- let's just do the assert explicitly where the logic calls for it (even if that means doing it in two places). Boolean params are bad news for readers of code: They're context-less at call sites, so readers have to go look up the function definition to understand what they mean.

[jeffmo]

Can you also add an error-test for <a[foo]></a[foo]> and <a['foo']></a['foo']>?

[syranide]

For sure, pushed!

[syranide]

Although <a[foo]></a[foo]> should have been enough seeing as you can't get to ['foo'] without the other syntax being supported (or at least producing a different error message).

[jeffmo]

Yea, I kind of just wanted to use the test as a way of documenting that we are explicitly not supporting this case

[syranide]

@jeffmo Sounds reasonable.

Thought; could rename parseXJSIdentifier to parseXJSNamespacedIdentifier and reintroduce parseXJSIdentifier as a call to the new parseXJSNamespacedIdentifier + throw on namespace. No redundant throw, it's prepared for (my) next PR without being especially messy (but perhaps a bit ugly if you want to enforce parseXJSNamespacedIdentifier returning a XJSNamespacedIdentifier). Or just wait? :)

[syranide]

FYI, the JSXTransformer PR can be landed before this esprima PR without any interference.

[jeffmo]

Lets wait on splitting out the namespacing (and by wait, I just mean put it in a separate follow-up PR stacked on top of this one -- you can put it out now if you're ready)

Let me pull this in and run our internal tests, then I think we're good to go if that's all set.

cc @benjamn as I think this might also need a corresponding update to your ast-types repo?

[jeffmo]

Ok one final thing: Can you run npm test and fix up all the lint errors there? The runner stops after the first error, so it's possible there may be a couple of test coverage errors as well that neither of us caught -- which we'd need to clean up as well

[syranide]

npm test was new to me, I've fixed all the lint errors.

Coverage complains: Uncovered count for statements (9) exceeds threshold (8)

I looked through the coverage report and all I found was a not-meant-to-be-reached assert of mine, which I can't cover in a test (obviously). Do you want me to increase the statements threshold (where?) or what do you prefer?