-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSX namespace names should not be considered expressions #54104
JSX namespace names should not be considered expressions #54104
Conversation
@typescript-bot test this |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at b22a671. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at b22a671. 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 b22a671. You can monitor the build here. |
Heya @weswigham, I've started to run the perf test suite on this PR at b22a671. You can monitor the build here. Update: The results are in! |
Good point - let's try that out in a follow-up PR on the top 100. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but get another review.
Also, might be good to have a test with an index signature like `ns:do-${string}`
@weswigham Here they are:
CompilerComparison Report - main..54104
System
Hosts
Scenarios
TSServerComparison Report - main..54104
System
Hosts
Scenarios
StartupComparison Report - main..54104
System
Hosts
Scenarios
Developer Information: |
@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. |
Perf run almost looks like this is visible in parse time, but it’s pretty noisy, and the only real addition is a |
@typescript-bot perf test this faster |
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at b22a671. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:Comparison Report - main..54104
System
Hosts
Scenarios
Developer Information: |
Fixes the regression in this comment, which was regressed by #47356 (and followup #53799), which mistakenly classified
JsxNamespacedName
nodes asExpression
s, which masked a ton of locations where they were effectively unhandled - the relevant location in the case of the regression beinggetLiteralTypeFromPropertyName
, which started returningerrorType
for all jsx namespace names (which perviously were correctly thea:b
string, thanks to the funky identifier we used to use).As an aside,
checkExpressionWorker
currently silently returnserrorType
for all unknown expression types - shouldn't this be a hardDebug.fail
calling out the unhandled case? This may have been more obvious had that been the case.