(feat) add svelte2tsx for better ts support #57

dummdidumm · 2020-05-04T17:55:08Z

This PR adds svelte2tsx to get better typescript support.

Still TODO:

There is one failing test for code actions. I think there is an edge case of source mapping there, the end of the replace range cannot be mapped. Not really sure how to deal with this. Change test to see if non-edge-cases work and then just leave it at that? see (feat) add svelte2tsx for better ts support #57 (comment)

To discuss:

Right now it is differentiated between JSX and TSX based on whether or not the user uses lang="typescript" or not. Advantage: User does not get type errors he does not want because he uses JS. Disadvantage: You don't get component name/prop typechecking. What to do? Maybe add a settings option to make this configurable by the user? the @ts-check comment can be used for that
~~I think the @ts-check comment at the top of script contents will not work anymore. Is this bad?~~ will work now

@halfnelson could you also have a short look and see I'm not doing anything funky there?

Related #11

Makes code more readable, no unnecessary interim stage

jasonlyu123 · 2020-05-05T10:41:09Z

I think @ts-check should be moved to the top of jsx in svelte2tsx. Typescript would then check js if it exists. As for per project check, user should use the checkJS option in jsconfig.

The lang/attribute setting is kind of weird to me though. Because it would change the entire file to ts, not just the script it annotated. I understand it's very hard to support multiple-languages in the same component. Maybe we should warn user that this is not supported. Or we could invent a new special comment/tag/detective that should be placed at the top of the file to specify ts-check and the language type of that file.

halfnelson · 2020-05-05T10:54:34Z

packages/language-server/src/plugins/typescript/DocumentSnapshot.ts

+                    tsxMap.sources = [uri];
+                }
+            } catch (e) {
+                text = '';


I had this approach and found that it gave too man angry red squiggles when the document is in a state where it won't parse.

https://github.com/halfnelson/svelte-type-checker-vscode/blob/svelte-native-support/server/src/DocumentSnapshot.ts#L50
In my plugin I stash the parse error so that when I get a call to get the diagnostics, I can check for the existence of the error and return the parse error (instead of treating the file as blank). Is the intention to let the svelte plugin point out the location of an parse errors?

Good point. At the moment there is no error at all shown if there are parsing errors. With your strategy this could be pointing to parser errors instead, as you said. I will look into it.

halfnelson · 2020-05-05T11:05:28Z

Looks good to me 👍 Regarding the failing test for code actions. I had a bunch of trouble with off by one errors, especially due to the fact that TS's lines start at 0 and characters at 1, and sourcemap starts at 1,1 (from memory), So there might still be a mapping conversion error in there.

dummdidumm · 2020-05-05T11:19:57Z

The lang/attribute setting is kind of weird to me though. Because it would change the entire file to ts, not just the script it annotated. I understand it's very hard to support multiple-languages in the same component. Maybe we should warn user that this is not supported. Or we could invent a new special comment/tag/detective that should be placed at the top of the file to specify ts-check and the language type of that file.

Sorry, I don't fully understand what you mean. Do you mean the current implementation is confusing? Or that it would be confusing if we would add a config setting?

I also wonder what's going on under the hood with if the user does not specifiy to use typescript, so the ending is jsx; but still svelte2tsx will create a tsx file. Guess I have to dig further into this.

jasonlyu123 · 2020-05-05T12:06:08Z

@dummdidumm what I think is weird is that the lang/type attribute on one script would change the entire file's script kind.

<script context="module">
let b = false;
b = 1;  // get type error here
</script>
<script lang="typescript">
let a = 1;
</script>
<button tabindex="-1" /> <!--get type error here -->

So I thought if it would better be if we have a new way to set the script kind at the top of the file. for example:

<!--lang=typscript-->
<script>
</script>

<ComponentA />

dummdidumm · 2020-05-05T12:21:31Z

Ah I understand. Yeah I think this new way would clear up confusion, but at the same time it could introduce new ones. As of now, all preprocessors look at lang or type attributes inside style / script to know what to do. So someone could ask "why do I need to add this comment at the top additionally?". Also, most users will only ever have one script block inside a component, and they would expect to have everything checked if they add lang="typescript" to one of the script tags - at least that is what I think.

jasonlyu123 · 2020-05-05T12:45:58Z

Fair point.
What about the user explicitly use two languages in the context and the instance script. which one gets priority? Also, should we warn them this is not supported?

<script context="module" lang="javascript">
</script>
<script lang="typescript">
</script>
<button tabindex="-1" />

dummdidumm · 2020-05-05T13:05:26Z

I would give typescript the priority since javascript is valid typescript but not the other way round. A warning is a good idea.

dummdidumm · 2020-05-05T16:19:00Z

Ok, I did test what happens if we use a script without typescript but with checkJs enabled -> it reports errors. So I would leave the logic as is for now: If the user does have lang="typescript" or the likes set on his script, the file will be treated as tsx, else jsx.

What will not work is adding //@ts-check to the top of the file. @halfnelson would you consider adding something like "if //@ts-check on top of the script, add this comment to the top of the tsx-file" to svelte2tsx? If not, we have to either say "ok this is not supported anymore" or deal with it ourselves.

dummdidumm · 2020-05-05T17:00:58Z

Ok I found the bug that produces the code actions error.

This script:

<script>let a = true</script>

Gets transformed to this:

<></>;function render() {
let a = true; /// <<---- additional ;
<></>
return { props: {}, slots: {} }}

export default class {
    $$prop_def = __sveltets_partial(render().props)
    $$slot_def = render().slots
}

The code has added a ; at the end of the line. The code fix wants to delete the whole line, which means it slips into the next line, which does not exist in the original, and boom. It seems like svelte2tsx adds a ; to the end of the script. And if the script closes within the same line as the last code AND that code has no ; at the end, then svelte2tsx does not add the ; in the next line but in the same line. The fix would be to always add that ; in a new line. This is a huge enough edge case for me that I'm ok with adjusting the test.

There is an edge case where it fails, but that edge case is edgy enough to adjust the test. See sveltejs#57 (comment)

- adjust completions (some workarounds for wrong mappings needed) - add more completions tests

dummdidumm · 2020-05-08T16:24:52Z

Quick update: I merged master into the branch and am now working hard on getting imports right for svelte components. Turns out that there are a lot of edge cases. Also I came across a major drawback of the svelte2tsx approach: Because the svelte compiler is used, intermediate states inside the template will almost always give parsing errors, so there is no code generated.

For example this:

<script>
</script>
<SomeComp

is not recognized as valid by the compiler, so I have to place the import at the right position "by hand".

EDIT: just had an idea how to simplify this. Doing this would mean to alway do the import below the script opening tag, not below the last previous import. But I think that's okay.

…ng back to script extraction

dummdidumm · 2020-05-09T08:04:39Z

Svelte imports are working now, without compromising the other imports.

So there is only one thing left to discuss: Ditch //@ts-check support and maybe add it again later or add it as part of this PR? What do you think?

jasonlyu123 · 2020-05-11T01:52:04Z

I would like to retain support for @ts-check and @ts-nocheck. If someone used checkJs in the compiler option, they might expect to be able to whitelist some file. And svelte2tsx doesn't move either of them to the top.

Is it possible that we add it to the top of tsx if we detect it for now? This detection can be removed later if svelte2tsx decide to support it.

@orta do you know if is there any public typescript API that can detect @ts-check and @ts-nocheck? I only found some internal. if not, is this regex ok?

/^\s*(\/\/[ \t\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff]*@ts-(no)?check$)/

the first characher set is just \s without linebreak and vertical tab

dummdidumm · 2020-05-11T09:22:45Z

I added //@ts-(no)check support, it's now ready for review and merge if accepted.

Some things could be refactored. For example I now need style tag info inside ts aswell, so we could move this extraction to the parent document. But I would like to keep these refactorings out of this PR because I fear it will get too big and will touch too many files.

jasonlyu123 · 2020-05-11T10:24:50Z

Find one issue, the template type check diagnoses needed to update if a dependent component is updated. But this seems needed quite some code to fix, I think, as you said this PR is getting too big, it could be done in another PR.

Overall this is working quite nicely!

orta · 2020-05-12T12:29:18Z

Perfect, I was about to recommend // @ts-check as the solution to your PR question. Reading PR now.

orta · 2020-05-12T12:32:29Z

@jasonlyu123 - no, no public API for that (it's a step in the parser for us)

orta

Looks great - a minor regex issue, but everything feels to be in place. Great work.

orta · 2020-05-12T12:33:53Z

packages/language-server/src/plugins/typescript/DocumentMapper.ts

+        result.line += this.nrPrependesLines;
+        return result;
+    }
+}


orta · 2020-05-12T12:35:09Z

packages/language-server/src/plugins/typescript/DocumentSnapshot.ts

+                      await new SourceMapConsumer(this.tsxMap),
+                      uri,
+                      this.nrPrependedLines,
+                  );


this is a lot happening on a single express, might be worth a refactor later

Absolutely true, one of the first things I'm gonna look into after this is merged.

orta · 2020-05-12T12:39:51Z

packages/language-server/src/plugins/typescript/DocumentSnapshot.ts

    }
 }
+
+// eslint-disable-next-line
+const tsCheckRegex = /^\s*(\/\/[ \t\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff]*(@ts-(no)?check)($|\n|\r\n))/;


This regex might needs a tweak, here's some examples: https://regex101.com/r/ZXpJNc/2as I think TypeScript is more liberal (any type of comment, and any whitespace after ) in accepting ts-check

You are right. I played around a little. Seems like it accepts the comment if

it is before the first line of code (so other lines with comments before it are ok)

must be @ts-(no)check

the comment which has @ts-(no)check can have any type of whitespace before it, but not other characters

what's coming after @ts-(no)check is irrelevant as long there is any kind of whitespace or line break, so this would be picked up, too: // @ts-check asdasd

orta · 2020-05-12T12:42:14Z

packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts

+            .map((diagnostic) => mapDiagnosticToParent(fragment, diagnostic))
+            .filter(
+                (diagnostic) => diagnostic.range.start.line >= 0 && diagnostic.range.end.line >= 0,
+            );


I wonder if this could accidentally remove diagnostics which come from setup around the specific file? It seems unlikely, but at least worth bringing up

I added this because in some rare cases the diagnostic could not be mapped correctly. But VSCode or the LSP (don't know which one) then seems to swallow all other errors aswell as soon as it encounters one diagnostic with negative lines. I will add a comment.

orta · 2020-05-12T12:43:11Z

packages/language-server/src/plugins/typescript/features/CompletionProvider.ts

            commitCharacters: getCommitCharactersForScriptElement(comp.kind),
-            preselect: comp.isRecommended,
+            // Make sure svelte component takes precedence
+            sortText: isSvelteComp ? '-1' : comp.sortText,


orta · 2020-05-12T12:43:44Z

packages/language-server/src/plugins/typescript/features/CompletionProvider.ts

        };
    }

-    private getCompletionLableAndInsert(comp: ts.CompletionEntry) {
-        const { kind, kindModifiers, name } = comp;
+    private getCompletionLableAndInsert(fragment: SnapshotFragment, comp: ts.CompletionEntry) {


irrelevant: getCompletionLableAndInsert has a typo

that is my fault lol

Np, I will change it 😄

Simon Holthausen added 3 commits May 4, 2020 19:48

(feat) add svelte2tsx for better ts support

752f42d

(refactor) throw out TypescriptDocument in favor of DocumentSnapshot

e5fbf35

Makes code more readable, no unnecessary interim stage

(fix) fix go to definition for svelte components

f001bc6

halfnelson reviewed May 5, 2020

View reviewed changes

(feat) add parser error to diagnostics

a4f272f

Simon Holthausen added 2 commits May 5, 2020 19:03

(fix) fix code actions, adjust test

ac63301

There is an edge case where it fails, but that edge case is edgy enough to adjust the test. See sveltejs#57 (comment)

Merge branch 'master' into svelte2tsx

98287d2

- adjust completions (some workarounds for wrong mappings needed) - add more completions tests

Simon Holthausen added 3 commits May 9, 2020 08:52

(fix) imports of svelte components WIP

c10a5cf

(fix) deal with svelte2tsx not being able to preprocess file by falli…

0488b67

…ng back to script extraction

(fix) don't import already imported svelte component again

5831254

Simon Holthausen added 5 commits May 11, 2020 09:54

(fix) don't provide ts-completions inside style tags

e1eaaf0

(fix) fix isBeforeOrEqualToPosition

5b2f67a

(fix) catch existing import suggestions earlier

6c589d1

(fix) reenable //@ts-(no)check support

f7f0cb8

(fix) fix lint errors

97eaaca

dummdidumm marked this pull request as ready for review May 11, 2020 09:20

Merge branch 'master' into svelte2tsx

a225f28

Simon Holthausen added 3 commits May 11, 2020 11:36

(fix) harmonize diffs in newlines for different OSses

53c3d00

(fix) harmonize newlines (next try)

a890cb3

(fix) harmonize line breaks for one more test

ae167e6

dummdidumm mentioned this pull request May 12, 2020

Overview of current features of language server + limitations #83

Open

47 tasks

Merge branch 'master' into svelte2tsx

f413439

dummdidumm mentioned this pull request May 12, 2020

Class completion for CSS #84

Open

(refactor) simplify completion code

c04c4bb

dummdidumm requested a review from orta May 12, 2020 08:28

orta approved these changes May 12, 2020

View reviewed changes

Simon Holthausen added 4 commits May 12, 2020 16:25

(doc) add explaining comment

8c80107

(fix) fix typo

7ced88a

(fix) fix regex and result extraction

8dbe8a1

(doc) more regex explanation

909c433

dummdidumm merged commit 6cb8d6e into sveltejs:master May 12, 2020

dummdidumm deleted the svelte2tsx branch May 12, 2020 15:12

dummdidumm mentioned this pull request May 12, 2020

Fixing reactive values in typescript #19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) add svelte2tsx for better ts support #57

(feat) add svelte2tsx for better ts support #57

dummdidumm commented May 4, 2020 •

edited

Loading

jasonlyu123 commented May 5, 2020 •

edited

Loading

halfnelson May 5, 2020

dummdidumm May 5, 2020

dummdidumm May 5, 2020

halfnelson commented May 5, 2020

dummdidumm commented May 5, 2020

jasonlyu123 commented May 5, 2020 •

edited

Loading

dummdidumm commented May 5, 2020

jasonlyu123 commented May 5, 2020 •

edited

Loading

dummdidumm commented May 5, 2020

dummdidumm commented May 5, 2020

dummdidumm commented May 5, 2020 •

edited

Loading

dummdidumm commented May 8, 2020 •

edited

Loading

dummdidumm commented May 9, 2020

jasonlyu123 commented May 11, 2020 •

edited

Loading

dummdidumm commented May 11, 2020

jasonlyu123 commented May 11, 2020 •

edited

Loading

orta commented May 12, 2020

orta commented May 12, 2020

orta left a comment

orta May 12, 2020

orta May 12, 2020

dummdidumm May 12, 2020

orta May 12, 2020

dummdidumm May 12, 2020

orta May 12, 2020

dummdidumm May 12, 2020

orta May 12, 2020

orta May 12, 2020

jasonlyu123 May 12, 2020

dummdidumm May 12, 2020

(feat) add svelte2tsx for better ts support #57

(feat) add svelte2tsx for better ts support #57

Conversation

dummdidumm commented May 4, 2020 • edited Loading

Still TODO:

To discuss:

jasonlyu123 commented May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfnelson commented May 5, 2020

dummdidumm commented May 5, 2020

jasonlyu123 commented May 5, 2020 • edited Loading

dummdidumm commented May 5, 2020

jasonlyu123 commented May 5, 2020 • edited Loading

dummdidumm commented May 5, 2020

dummdidumm commented May 5, 2020

dummdidumm commented May 5, 2020 • edited Loading

dummdidumm commented May 8, 2020 • edited Loading

dummdidumm commented May 9, 2020

jasonlyu123 commented May 11, 2020 • edited Loading

dummdidumm commented May 11, 2020

jasonlyu123 commented May 11, 2020 • edited Loading

orta commented May 12, 2020

orta commented May 12, 2020

orta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dummdidumm commented May 4, 2020 •

edited

Loading

jasonlyu123 commented May 5, 2020 •

edited

Loading

jasonlyu123 commented May 5, 2020 •

edited

Loading

jasonlyu123 commented May 5, 2020 •

edited

Loading

dummdidumm commented May 5, 2020 •

edited

Loading

dummdidumm commented May 8, 2020 •

edited

Loading

jasonlyu123 commented May 11, 2020 •

edited

Loading

jasonlyu123 commented May 11, 2020 •

edited

Loading