Fix poor error span for unclosed JSX tags in the presence of whitespace/comments #37419
Conversation
|
|
||
| let y = < Baz >Hello | ||
| ~~~~~~~~~ | ||
| !!! error TS17004: Cannot use JSX unless the '--jsx' flag is provided. |
There was a problem hiding this comment.
You can get rid of these errors by adding // @jsx: react at the top of tests/cases/compiler/errorSpanForUnclosedJsxTag.tsx
There was a problem hiding this comment.
Thanks for helping me sort this out.
src/compiler/parser.ts
Outdated
| else { | ||
| parseErrorAtRange(openingTag.tagName, Diagnostics.JSX_element_0_has_no_corresponding_closing_tag, getTextOfNodeFromSourceText(sourceText, openingTag.tagName)); | ||
| // We want the error to span only property identifier "Foo.Bar" e.g in < Foo.Bar >... | ||
| const tag = openingTag.tagName as any; |
There was a problem hiding this comment.
Why is this as any necessary?
There was a problem hiding this comment.
The compiler was complaining about missing properties on tagName. I tried using some types from the types.ts file but that only partially solved the warnings. With your suggested changes implemented, this is not needed anymore and I have removed it.
src/compiler/parser.ts
Outdated
| parseErrorAtRange(openingTag.tagName, Diagnostics.JSX_element_0_has_no_corresponding_closing_tag, getTextOfNodeFromSourceText(sourceText, openingTag.tagName)); | ||
| // We want the error to span only property identifier "Foo.Bar" e.g in < Foo.Bar >... | ||
| const tag = openingTag.tagName as any; | ||
| let start = tag.pos; |
There was a problem hiding this comment.
I think a better/simpler way would be to just write
const start = skipTrivia(sourceText, openingTag.tagName.pos);There was a problem hiding this comment.
I have changed it to use skipTrivia instead. Thanks a lot for the review.
src/compiler/parser.ts
Outdated
| start = tag.end - tag.escapedText.length; | ||
| } | ||
|
|
||
| parseErrorAtRange({ pos: start, end: tag.end }, Diagnostics.JSX_element_0_has_no_corresponding_closing_tag, getTextOfNodeFromSourceText(sourceText, openingTag.tagName)); |
There was a problem hiding this comment.
We already have a parseErrorAt which takes a start/end pair and avoids an extra allocation.
There was a problem hiding this comment.
I have changed this as well to use parseErrorAt. Thanks for reviewing :)
|
The current approach here hits all the right places - I just think there are a few fixes to take care of and once those are addressed, we can merge it in. |
Let me work on implementing your feedback. Thanks for the review. |
Co-Authored-By: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
|
hello @DanielRosenwasser , I worked on your feedback. Issue now is that all tests pass locally for some reason the pipeline fails due to some npm error. |
| @@ -0,0 +1,12 @@ | |||
| // @jsx: react | |||
| import React from "react"; | |||
There was a problem hiding this comment.
You can replace this with declare const React: any to avoid import errors, especially since this is a syntactic test.
|
I'm not sure why this was running into CI errors but they seem to be passing now! |
Not sure as well. Thanks a lot for the help. I only started looking at the TS compiler code last week :) |
2 participants
Fixes #37306