-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add parsing-only support for typescript #154
Conversation
src/main/java/dev/blachut/svelte/lang/parsing/html/SvelteHtmlLexer.kt
Outdated
Show resolved
Hide resolved
if (toolId == "UnnecessaryLabelJS") { | ||
return element.textMatches("$") | ||
} | ||
if (toolId == "BadExpressionStatementJS") { |
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.
I didn't see problems from this inspection in JS, is this strictly TS related or did I miss something?
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.
it fixes useless warning for $: name
override fun buildTokenElement(type: IElementType) { | ||
// there are too many places that uses element type JSElementTypes.REFERENCE_EXPRESSION, | ||
// so use the new one only for the specific references | ||
return super.buildTokenElement( |
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.
Interesting, I tried to use buildTokenElement
but had some problems with it, because not every marker.done
goes through this method.
I found one example that does not work that way:
https://svelte.dev/repl/65fc4b475b884dcba414139848ff02ef?version=3.29.0
const count = writable(0);
const x = { $count };
console.log(x); // Prints { $count: 0 }
Anyway this looks like weird side effect of stores implementation and not a feature so I guess it's okay to ignore it for now.
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.
yes, I think so. Unfortunately, we cannot get the token text using the other implementation because it is called after "advance()" method
val psiFile = element.containingFile | ||
if (psiFile?.fileType !is SvelteHtmlFileType) return null | ||
if (psiFile !is XmlFile) return null | ||
val scriptTag = JSUtils.findScriptTagContent(psiFile) |
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.
Svelte can have two script tags and they could have different languages, this finds first one, is it okay?
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.
I think we can live with it as is. I will try to find a better strategy but I am not sure
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.
We way we handle this in the language server is to check script and then module script and pick the first lang/type tag we find, so if someone chooses TS in the script tag he will automatically use it in the other one as well - no language mixes.
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.
Seems like a good balance 👍
import com.intellij.psi.xml.XmlFile | ||
import dev.blachut.svelte.lang.SvelteHtmlFileType | ||
|
||
class SvelteElementResolveScopeProvider : JSElementResolveScopeProvider { |
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.
THB I don't understand what this class does
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.
Excellent, thanks so much. I left some notes, please respond in your spare time.
I'll go ahead and add completions for script lang attribute similar to those for style tag, and create next release.
#32 can be closed with this, right? LSP support is not really urgent now I think.
@tomblachut I am 100% sure that we still need to provide the integration with ts service but yes, it isn't a critical task for now. |
Okay |
Added parsing support for typescript in script tags with
lang="ts"
There are some limitations of the implementation:
2. Auto-imports don't work (only imports by quick fixes) // I will take a look, most likely it can be easily fixedfixedTypeScript
as the content language unlike regular script tags that use an extension of js languageSvelteJSLanguage