Support xml namespace prefix for JSX elements and attributes

[tomjw64]

I got tired of the VSCode TypeScript checker complaining about JSX that React, Babel, and the JSX spec consider perfectly fine, so I thought I'd check to see this feature has been requested in the past and make a PR if so. Looked to be two issues requesting the support. I haven't contributed to TypeScript before, so I've probably done this all wrong, but I'm willing to put some more of my personal time into this to get it right. Thanks for your time.

Checks

• There is an associated issue in the Backlog milestone (required)

No, but here's a quote saying there's a chance that a PR could be taken (@RyanCavanaugh on March 7, 2016):

We'd be happy to add this support if there's some real-world use of it. If it's just for the sake of some other tool that supports it (i.e. there are no actual downstream users), we'd take a PR but wouldn't implement it ourselves.

• Code is up-to-date with the master branch
• You've successfully run gulp runtests locally
• There are new or updated unit tests validating the change

Fixes #11833, #7411

Commit message:

Just as with the - character, : is now also treated specially in JSX
element and attribute names, but is only allowed a single time, and not
at the beginning or end of the name, as is specified in the JSX spec.
All tests in jsxInvalidEsprimaTestSuite still fail, but for slightly
different reasons now. Two lines in jsxEsprimaFbTestSuite were
uncommented as they included elements with namespaces, and they now pass
without error.

[msftclas]

All CLA requirements met.

[4r7d3c0]

hey man GJ do you know if it'll fix this
#28905

[tomjw64]

@4r7d3c0 It will not. Sorry!

[Jack-Works]

Just for curious, what will the type check semantics for JSX namespace? Does it checked as a whole identifier like JSX.IntrinsicElements.element['namespace:attribute'] or JSX.IntrinsicElements.element.namespace.attribute? The latter one seems better to me and more extensible...

[tomjw64]

@Jack-Works Good thoughts. My guess would be that it checks the whole identifier, but I didn't include any tests for that. I'll look into the details of this when I have some time.

[Jack-Works]

Hi! My PR #37455 has been merged and it changed the identifier scanner to add JSX hyphen and JSX namespace support. (By a second parameter identifierVariant) You may want to rebase it and check it out
😆

[Jack-Works]

Why close it? 🤔

[tomjw64]

Oh, I thought you meant your PR had fixed the same issue. I'll reopen it, then.

[sandersn]

The reviewers list isn't working, but @weswigham and @DanielRosenwasser , you might want to look at this too.

[DanielRosenwasser]

As it stands this PR needs

1. Some semantic analysis/errors when a namespaced name does come up, especially when the JSX target is react.
2. Some tests for emit behavior when the JSX target is react.

I think @RyanCavanaugh is a better person to speak to the expectations of this PR, and I would hold off on further work until there's further clarity.

[weswigham]

Some tests for emit behavior when the JSX target is react.

Unless I'm mistaken, it should get compiled to something like <Tag with:ns={value}> becomes

React.createElement(Tag, { "with:ns": value });

based on how we parse it? Which seems.... ok. Even if there's no spec stating as such, I'm not sure what else would be reasonable.

[Jack-Works]

Does react ok with this emit behavior? For example have a try to use it to render svg with svg ns

[andrewbranch]

The Babel React transform issues the message:

Namespace tags are not supported by default. React's JSX doesn't support namespace tags. You can set throwIfNamespace: false to bypass this warning.

[Jack-Works]

@andrewbranch How do you think the type of <element namespace:attribute /> should be defined by JSX.IntrinsicElements.element.namespace.attribute?

[andrewbranch]

I really doubt that it makes sense to use a nested property. I assume the property name on the element’s props type would just be "namespace:attribute".

[Jack-Works]

I really doubt that it makes sense to use a nested property. I assume the property name on the element’s props type would just be "namespace:attribute".

By making it nested, it is possible to reuse attributes. for example:

interface IntrinsicElements {
    element: {
          namespace1: attributes // type reference
          namespace2: attributes // type reference
    }
}

Without this, I have to write "namespace1:attr1": attributes['attr1'], "namespace2:attr1: attributes['attr1'] because ts can't calculate string literal in type level

[victorandree]

@Jack-Works What would you propose namespaced tags should emit or typecheck against?

IMHO, understanding that : is a namespace separator and not just an allowed part of JSX identifiers is flying a little bit close to XML namespaces, without fully getting there.

I'm not sure what can be emitted (or typechecked against) except plain identifiers. Wouldn't nested types require nested props (React.createElement(Tag, { "with": { "ns": value } });)? How would you deal with a namespaced intrinsic tag?

I'll note that namespaced JSX is useful to generate XML from JSX, using a package like jsx-xml. I would like this to work:

import { JSXXML } from 'jsx-xml';

const xml = <ns:tag ns:attr="foobar">foobaz</ns:tag>;

With {"compilerOptions": {"jsx": "react", "jsxFactory": "JSXXML"}}, I'd expect this to emit:

import { JSXXML } from 'jsx-xml';

const xml = JSXXML("ns:tag", { "ns:attr": "foobar" }, "foobaz");

[Jack-Works]

Maybe we should leave the type check behavior until the real world usage

[tomjw64]

Is there any work I can do on this PR regarding the discussions above? It is difficult for me to discern a consenus about what is needed.

[tomjw64]

I have removed the check that the code is up to date with master since it has been long enough there are conflicts now. I still would be willing to move forward on this PR in my free time if there is a clear path.

[andrewbranch]

I’d like to hear some more detailed use cases for namespaced attributes so we can make sure we’re not boxing ourselves into a type checking behavior we’ll be unhappy with. We can’t just ignore the type checking behavior for now, because if I understand correctly, merging this as-is would allow libraries to augment JSX.IntrinsicAttributes with namespaced properties, and type checking those would behave the same way as non-namespaced properties. I think that’s probably what we want, but I haven’t really heard how this feature will be used, other than from @victorandree on generating arbitrary XML.

[tomjw64]

@andrewbranch Added all the aforementioned tests, and it looks pretty decent to my eyes. jsx: react yielded what seemed to be reasonable output and the type checking behavior with intrinsics matched your example. CI seems okay with everything. Anything missing?

[andrewbranch]

This looks great! The only questions I’d like to get feedback on from the team are:

1. Should <Upcase:component /> have a specific error message explaining that that construct is a grammar error, or is Cannot find name 'Upcase:component' sufficient?

2. Should we do something clever with the parse or emit of that construct to avoid emitting unparseable JS? Currently the emit is

   React.createElement(Upcase:component, null)

   which is invalid syntax. I think you could make the argument that TypeScript without parse errors should ideally produce JavaScript without parse errors.

[tomjw64]

Great thoughts. This work parallels the work to allow the - character in jsx identifiers. I looked around and found this function in src/compiler/utilities.ts:

export function isIntrinsicJsxName(name: __String | string) {
    const ch = (name as string).charCodeAt(0);
    return (ch >= CharacterCodes.a && ch <= CharacterCodes.z) || stringContains((name as string), "-");
}

which special-cases jsx names with the - character, perhaps to address the same points you brought up. If anything with : was parsed as an intrinsic, just like anything with -, then I think it might address both of these concerns. Thoughts on such a change?

[tomjw64]

While looking around, I also found this in src/compiler/checker.ts (I would link, but GH says it's too large), which seems relevant. This comment will no longer hold true if this PR is merged as-is.

/**
 * Returns true iff the JSX element name would be a valid JS identifier, ignoring restrictions about keywords not being identifiers
 */
function isUnhyphenatedJsxName(name: string | __String) {
    // - is the only character supported in JSX attribute names that isn't valid in JavaScript identifiers
    return !stringContains(name as string, "-");
}

I may have to look into how this might affect things if this no longer holds, although I'm not really sure what a "known property" is.

[andrewbranch]

Great find on isIntrinsicJsxName. I didn’t realize that’s what happened for things like <Component-name />, but it makes total sense to me that a colon would have the same effect there.

isUnhyphenatedJsxName in checker.ts is what allows custom/arbitrary hyphenated attributes (e.g. data-whatever="yes") to appear on JSX elements without error: https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4AoctCAOwGd4ATYANzgF44AeZtlKYCgC0jCEjo0YSAB7AG7AEQwoAVyQK4AegB8FIA. I don’t think the function needs to change, but the wording of that comment could be updated.

[tomjw64]

I made the adjustment on isIntrinsicJsxName. Now we get parseable JS in the React test case even when using an capitalized identifier that includes a colon.
Thanks for the advisement on isHyphenatedJsxName. I have adjusted the comment on that function, but left the code there unchanged.

[andrewbranch]

👍 and now we wait until the release-4.1 branch is cut from master and hope there aren’t too many merge conflicts in the meantime

[andrewbranch]

From my quick glance at this function’s usage, it seemed to me like the note about identifier validity isn’t actually relevant. On second thought, I would probably just remove these comments since they’re subtly misleading about the purpose of the function. The name and one-line implementation of the function are sufficiently self-explanatory.

[SoloJiang]

How can I use this feature, it is still reporting an error in version 4.1.2：

[andrewbranch]

@SoloJiang it is / will be in 4.2.