feat(7411): JSX namespaced attribute syntax not supported #47356
Conversation
|
The TypeScript team hasn't accepted the linked issue #7411. If you can get it accepted, this PR will have a better chance of being reviewed. |
There was a problem hiding this comment.
Can we get tests with more variety of odd namespace names in both tag name and property name positions? Like Component:foo or some-thing:bar? Specifically, in the case of the first one, I'd like to know if it tries to somehow lookup a Component:foo local variable (which'd be odd) for the type of the tag. (You could technically make this with an assignment to globalThis["Component:foo"], except the lookup for the tag probably has no shot of resolving at runtime - do namespaced custom tags even work?) which I'm down with just looking up intrinsics named svg:path and so on and props named ns:prop and so on, so that part of the checking seems fine. But do we need errors when a react-ish emit mode is set? react doesn't use namespaces as far as I'm aware. Maybe this should be flagged in some way because of that? (it's flagged in babel afaik)
Additionally, ping @RyanCavanaugh for the go/no go on this; we didn't want em for a long time, but I guess some framework (vue2?) uses the namespace syntax now, even though you have to silence an error in babel to use it.
|
@weswigham Thanks for the review.
I'm not sure I fully understood the requested changes. Do you mean something like this? Or do you mean that I need to define global types to check name resolution? // @jsx: preserve
// @filename: a.tsx
<A:foo></A:foo>;Could you provide some examples that I can use as tests? |
Yeah, with both capital-letter-starting and dash-containing names. (The first being an indication of function components and the later indicating non-identifier builtins). For checking intrinsics, adding namespaced dashed names to intrinsic tag names should be fine. For the capital-letter variant, I don't think there's any reasonable way to check it or even transpile it, after all |
Shouldn't we transpile it as |
|
No - only intrinsic names do that, and only names starting with a lower case character are intrinsic names. |
|
@weswigham Oke. Should TS parse it as |
|
Mmmm, checker is better for error recovery, usually. |
a-tarasyuk
force-pushed
the
feat/7411
branch
2 times, most recently
from
ac7f3d6 to
2ff0115
Compare
a-tarasyuk
force-pushed
the
feat/7411
branch
2 times, most recently
from
eac179e to
888f234
Compare
@RyanCavanaugh What do you think about JSXNamespacedName support? |
There was a problem hiding this comment.
This looks OK to me, but I'm going to kick it over to @RyanCavanaugh for final approval and merging after community tests have come back, since it's up to him if this much support for namespace names is actually what we wanted (or if we just wanted better parser recovery).
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 23379dd. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 23379dd. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 23379dd. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 23379dd. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 23379dd. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @weswigham, the results of running the DT tests are ready. Branch only errors:Package: fbt |
|
@weswigham Here they are:
CompilerComparison Report - main..47356
System
Hosts
Scenarios
TSServerComparison Report - main..47356
System
Hosts
Scenarios
StartupComparison Report - main..47356
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
It's not really clear to me whether this was targetted just at attributes or also element names. The spec does allow namespaced names as far as I can tell so TypeScript should allow it. And considering Either way, can somebody take a look at the test added in https://github.com/microsoft/TypeScript/pull/53784/files since that should resolve the element type to |
Fixes #7411
Fixes #47219
JSX Spec