Fix default export return type

[firefish5000]

Currently the tsx we generate seems to loose type information and generate invalid d.ts files.
The type loss seems to be due to the use of AConstructorTypeOf.

This currently affects the svelte-vscode package. In a non-mythical B.svelte I get

<script lang="ts">
import A from './A.svelte'
const a = new A({})
//             ^ `(alias) new A(...args: any[]): A new A() = ()` 
//                Type resolves incorrectly.
//                Attempts to autocomplete target/props in the brackets will fail.
</script>
<A></A>
<!-- ^ JSX is fine. Resolved properly! Hurray!! -->

I did not understand why we are using virtual functions here to begin with (or how to make it work in this case) so I eliminated it and just extended Svelte2TsxComponent directly. This fixes the issue above along with the problems I was having writing my own package which depends on svelte2tsx.

I have yet to update the tests. This invalidates all svelte2tsx sample tests.
I still need to double check the types I generate. Especially the types for events/slots. (should these be partial?) Will try to check what the jsx thinks it is and act accordingly.

For anyone who wants to test the vscode svelte plugin with this change. Here is a working snipit to install it. Other than new Component({}) now being properly typed in svelte files, I believe there is no noticeable change.
If there are any new issues, I imagine they will probably be blatantly obvious like... no types working for svelte files, all svelte imports having wrong types, or svelte-check not working at all.

#!/bin/bash

# ensure we die on error
set -e

# Fetch modified repo
# git clean -fxd
# git checkout -- ./
# git pull

# optionaly rename the vscode extension so you do not have to remove the existing extension, just disable it.
sed --in-place -e 's/^\(\s*"name": \)"[^"]*"/\1"svelte-vscode-unstableer"/' packages/svelte-vscode/package.json

# here is the deal. yarn is not working for us at all. It hates local packages as much as it loathes me, and one of the tasks of your github workflow is to kill it anyways....
rm --force yarn.lock package.json packages/svelte-vscode/yarn.lock

# Build and pack our changes
pushd packages/svelte2tsx
npm i
npm run build
npm pack
popd

# Install changes into language-server
pushd packages/language-server
npm i
npm i ../svelte2tsx/svelte2tsx-0.1.4.tgz
npm run build 
npm pack
popd

# And the new language-server into svelte-check
pushd packages/svelte-check
npm i
npm i ../language-server/svelte-language-server-0.10.3.tgz
npm run build 
npm pack
popd

# And the new language-server into vscode
pushd packages/svelte-vscode
npm i
npm i ../language-server/svelte-language-server-0.10.3.tgz
# FIXME: this needs to go in devDeps or somewhere
npm i --no-save js-yaml
npm run build
npx vsce package
popd

# undo the wierdness
git checkout -- ./

After running the above, you can install the new vscode extension via

# assumes vscode is installed as the `code` binary.
code --install-extension ./packages/svelte-vscode/svelte-vscode-unstableer-0.5.0.vsix

Additionally, each of the other packages have their own .tgz file that you can install for testing as well. such as packages/svelte-check/svelte-check-1.1.0.tgz

Despite the fact it is intended as a fix, this is probably a breaking change... for some project somewhere.... just like it breaks our tests

[jasonlyu123]

I think the reason for the virtual helper function is to transform it into a valid JSX file when it's a js component. I tried it and it would break definition file generation. Maybe we should explore if there's any downside of only transforming it this when it's a typescript component.

[firefish5000]

@jasonlyu123 alright. I updated the install sometime within the last hour btw. yarn was giving me hell before. new script has been tested with multiple runs on a linux machine.

I am new to this ecosystem. I am curious as to why we need to generate valid jsx. I know the tsx is used for type checking... I have no clue why we would need to generate valid jsx though. And furthermore am lost as to why we are generating declaration files from jsx we generate and not just generating tsx to begin with. And if the virtual functions are only declared in typescript, in my mind, makes the "jsx" we generate tsx to begin with, furthering my confusion.

Making this change conditional is trivial enough. But I am lost as to why we do not just strictly generate tsx only.

[firefish5000]

Well, I guess there is no point breaking a working feature. The theory that they accidentally coded in a way that spits out pure jsx, despite the namesake, is probably heavily misguided. I will try the top 2 answers from the link I posted describing the source of the issue

That said, I tested pure svelte as well now and saw no regressions in vscode support. Though as you said, it is most certainly generating tsx now though (with valid declarations). Which absolutely cannot be given a jsx extension since that won't compile, run, or have any declarations

[dummdidumm]

We cannot compute a tsx file if the user is only using js inside Svelte because he then will get many type checking errors he does not expect/want/might not even be able to fix because he cannot type cast.

That said, I have no problems adding this kind of transformation for ts users. We need something like this anyway as soon as we start adding generics. #437 also adds some kind of extends Svelte2tsxComponent transformation. Note in there the call to render to prevent a unused function diagnostic warning.

You could also try to alter the AConstructorOf usage for the createSvelte2TsxComponent function, maybe it's possible to type it so that it's specifically tailored towards the class.

[dummdidumm]

Yes this is on purpose. Image you have some dispatcher mixin imported into the component which emits events as well. There is no way to find out from just looking at the Svelte file.
Also this code contains an error, the generic is not closed (>) if strictEvents is true.

[firefish5000]

Also this code contains an error, the generic is not closed

I am counting an equal number of opening and closing tags. <> + (strict events ? `` : <<>>)

[dummdidumm]

Yeah sorry, my bad 😄

[firefish5000]

I understand we need to avoid creating types for the component itself, like props, events, slots. But those are already not generated in the render function, which this does not touch.

From my perspective, the declarations we get via the shims are for the generic structure of a svelte component. These types are always received via the shims, jsx or not. (and if they are not, then I am lost as to why we generated a jsx file with a bunch of non-existent functions)

These types are unrelated to the declarations we need to avoid creating for the structure of the individual component we are shimming. That is, the type of props, events, slots, getters/setters, context. Most of the code that controls their types is in the render function, so just using its return type should be valid from my understanding.

But breaking syntactically valid jsx generators is not my end game. I think I found how to dresses up the class extends function thing in a way typescript likes.

[dummdidumm]

The thing is we cannot use types anywhere in the transformed JSX, because you cannot use types in JSX, that's just invalid syntax.If you want these types to mean something you gotta treat the whole file as tsx and then the unwanted errors appear for js users. That's the reason why there are so many functions in that shim file, it's kind of a workaround to still get proper type inference.

[firefish5000]

I understand all of your arguments except

errors appear for js users

Who are the js users that are using the jsx we generate? I know there are people writing js in svelte, but they are not using the jsx output of this program to the best of my knowledge. The only consumers of svelte2tsx output I am currently aware of is svelte language server, and things that use it (svelte-check and vscode). Which I believe all include typescipt and do not care if we output tsx since they are doing type checking anyways (via reading the shims which are 100% non js, typescript files).
That said, I updated the code. It now only touches the shims and seems to have fixed the issue. And since it only touches shims now... all test pass!

[dummdidumm]

Great to hear that! Looks good 👍

To answer who is using the jsx: The language server, which is used by the vscode plugin and svelte-check. The language server uses the typescript service but that service also can deal with JS files and will NOT throw type errors at you if pass in a JS file. That service can deal with a mix of d.ts / .js / .ts without problems. Fun fact: The same typescript service that powers our language server is the one which is used by VSCode for both JS and TS files (although a different instance of it, of course).

[firefish5000]

This is you codebase so you would know better than me who literally is only familiar with 2 files but... are you sure there is an instance where language-server expects and reads the output of svelte2tsx as jsx?
And are you sure that moving the shims (along with the typescript syntax) into the generated tsx file would cause typechecking to become more strict on svelte js files?
From what I can tell via blind usage of the previous commit, neither of those are true. It treats the output as tsx 100% of the time, and the reason it doesn't complain with js components is because we explicitly code it to generate non-strict typings (via injecting partial and any wrappers) when the component is written in js syntax. That is, tsx + partial and any = non-strict checks we run on js components

Not trying to change how it works. Just trying to understand the current ecosystem

(actually I did not try importing plain js files.... just js svelte components... is that what actually would break? I am running out of ideas of what to write to try to discover the break)

[dummdidumm]

There is all the time. There is a scriptKind property precisely to deal with the distinction of TS and JS files, to in turn treat the generated output as either TSX or JSX. That scriptKind is used in the module loader to accomplish exactly that. So to the TS service which we offload most of the work (including diagnostics) to, a Svelte-JS file is a JSX file, and a Svelte-TS file is a TSX file. And the TS service then does its internal handling and treating JS files differently.

Things that break:

<script>
   let a = true;
   a.methodCall(); // <-- will not error in JS files
   let b: boolean = false; // <-- should error "cannot use type annotations in JS files", but it will not if it's treated as TS
</script>

[firefish5000]

That code is also happy and sad in all the right places for me....
I would be so much less confused if the plugin compiled from the previous commit just died spectacularly with sparks everywhere because I fed it tsx syntax with script-kind of js/jsx.

[dummdidumm]

They do not appear because diagnostics which cannot be mapped/have negative lines are filtered out. So it's wrong, but we filter it out so noone sees it + the TS service is smart enough to still get everything else right - but I wouldn't want to rely on that. The "cannot use type annotations in JS files" will occur in other places because the module load part which I linked still things this is JSX, so yeah this will not break directly in this case. But still, I don't want to rely on TS service getting it right regardless of invalid syntax + swallowing errors.

Btw is this PR ready to get merged? It's still in draft mode. What about the TODO comments in there?

[firefish5000]

It should be safe to merge now.
But I was planning to walk through the code and figure out what other uses of AConstructorTypeOf are resolving. While it is safe to leave them alone, I would assume that anything using it could benefit from having the constructor's arg types properly resolved.

[firefish5000]

Removed the todo notes. I gave them a glance over, but the other shims did not seem like they would yield any obvious benefits from improving the constructor type, if that was even possible for them.