feat: Project reference support

[jasonlyu123]

#2148

It might make sense to make this part of the svelte-check v4 breaking changes. The way files are assigned to tsconfig needs to change, #1234 also needs that. And languageServiceHost.getParsedCommandLine also can't use the API we used to parse to tsconfig so it's also related to #1976. But we could also argue that assigning a file to a different tsconfig is already a breaking change. And we're unlikely to cherry-pick changes back to v3 so probably safer to make this v4 feature.

There are a few main changes:

1. project management. the open document is assigned to a service containing the document as a project file. This is actually the important part. The files need to be loaded with referenced tsconfig, Otherwise, TypeScript will skip the check. One problem with this is that you'll need to run svelte-check multiple times with different tsconfig. Should we introduce an option similar to tsc's --build?

2. module resolution only has one step now. No try ts first and then try custom logic later because the project-reference-redirect thingy uses the language service host and therefore it always uses svelte-sys.

3. Don't force that many tsconfig. But one question is should we still check incompatible compiler options? And should there be default a target? Removing the target default breaks a lot of our tests and might confuse a lot of users.

4. if an open file is not a project file for any tsconfig, it'll be treated as if there isn't a tsconfig.

5. update-import now run with forAllService because the default project assigns changes. Our update import doesn't block VSCode from actually renaming the file so sometimes the request is after the project files update, sometimes it is before. So using both oldPath and newPath doesn't reliably get the correct service and might unnecessarily create a default service. If it's folder rename it also won't work.

We could probably revert 3 and 4. 4 does help with catching some problems with project references. Since only the project files for the referenced project are assigned to the project.

While writing tests for this I also run into some problems with the existing test. Two are existing flaky tests and one just does not test anything because the assertion error is swallowed. Another change is that while updating the document, the getSnapshot method also unnecessarily opens a new service if it belongs to multiple services. If we decide not to merge this, I'll separate the fixes into separate PRs.

[dummdidumm]

Nice work!

One problem with this is that you'll need to run svelte-check multiple times with different tsconfig. Should we introduce an option similar to tsc's --build?

Can you elaborate on that? I don't fully understand? In which case would you need to run svelte-check multiple times, and how would you need to run it? And how would that differ to what you can do today?

module resolution only has one step now. No try ts first and then try custom logic later because the project-reference-redirect thingy uses the language service host and therefore it always uses svelte-sys.

Just curious: Is this is a nice side effect of this work or a necessary thing? This may even speed up things a little, right? Since we're not going to do it two times for Svelte files.

Don't force that many tsconfig. But one question is should we still check incompatible compiler options? And should there be default a target? Removing the target default breaks a lot of our tests and might confuse a lot of users.

Looking at the changed code there isn't any change to the tsconfig options? Did you revert that to make it separate?

Another change is that while updating the document, the getSnapshot method also unnecessarily opens a new service if it belongs to multiple services.

Can you give an example? How often will that happen? What would the new service be? How bad would that be for performance, or could that even introduce bugs?

[jasonlyu123]

In which case would you need to run svelte-check multiple times

Take our repo for example. The root tsconfig is a "solution style" tsconfig which includes nothing and is meant to load language-server and svelte-vscode. if you run tsc with this tsconfig, nothing will be built. You need to either run tsc twice in language-server and svelte-vscode, or run tsc -b with the root tsconfig. Maybe there are already people running it twice with the current svelte-check version since you can pass in an arbitrary tsconfig path.

Is this is a nice side effect of this work or a necessary thing

More like "a nice side effect". We'll need the compiler host created by TypeScript to redirect project reference .d.ts to the corresponding source file which means a lot of .ts files will go through the svelte-sys route anyway.

Looking at the changed code there isn't any change to the tsconfig options

If there is tsconfig the forced option won't override the tsconfig. The parseDefaultCompilerOptions is the old behaviour and overrides some TypeScript defaults. I should remove some unused logic to make that clearer.

the getSnapshot method also unnecessarily opens a new service if it belongs to multiple services

I found that with our test because the update-import test keeps timeout after the update-import change. I forgot to mention that in the PR description, adding that now. Some test files belong to multiple tsconfig because we didn't exclude them from the root tsconfig(test/plugins/typescript/testfiles/tsconfig.json). During the program creation of the root tsconfig, TypeScript asked for the snapshot for those files because they're project files. Our getSnpshot logic then looks at the files and assigns them to the nearest tsconfig. ex.
preferences/code-action.svelte to preferences/tsconfig.json. It probably doesn't happen that often in user setup but it does speed up our test a few seconds.

[dummdidumm]

During the program creation of the root tsconfig, TypeScript asked for the snapshot for those files because they're project files. Our getSnpshot logic then looks at the files and assigns them to the nearest tsconfig.

Ok so just be clear: You're saying you found a somewhat unrelated bug/performance issue and fixed that in this PR? I first understood it as "this PR has this drawback because it now does it", but it seems I understood wrong and it's actually the other way around?

update-import now run with forAllService because the default project assigns changes. Our update import doesn't block VSCode from actually renaming the file so sometimes the request is after the project files update, sometimes it is before. So using both oldPath and newPath doesn't reliably get the correct service and might unnecessarily create a default service. If it's folder rename it also won't work.

This was something that could happen before, too, but now can happen more often because we're possibly doing it for multiple services? Why would it unnecessarily create a default service?

Don't force that many tsconfig. But one question is should we still check incompatible compiler options? And should there be default a target? Removing the target default breaks a lot of our tests and might confuse a lot of users.

What about only setting target if we see it's either a very old one that likely breaks, or is not set at all?

[jasonlyu123]

You're saying you found a somewhat unrelated bug/performance issue and fixed that in this PR

Not a drawback but just explain why the change was made. Didn't remember the exact problem when I wrote the previous comment. Now I remember it is related to project references. ex: when a file is loaded in referenceA because of an import but is a project file of referenceB. This will cause the referenceB project to open while it may never be used.

Why would it unnecessarily create a default service

It is more related to the project assignment issue. Because now the client-opened file needs to match the "include" and "exclude" of the tsconfig otherwise we'll be treated as if it doesn't have any tsconfig. The project assignment change can be a separate PR but because project references also need this "include"/ "exclude" match check. So I put it in here as well but it could be separated later. Having it might catch some problem in project reference but doesn't have a dedicated test.

The referenced tsconfig needs to open a dedicated service really makes the thing a lot more complex 😅.

What about only setting target if we see it's either a very old one that likely breaks, or is not set at all?

The default is ES5 in typescript 5.5. Should we default it to ES2015? And if it's explicitly set to ES5, do we force it to be ES2015?

[dummdidumm]

The default is ES5 in typescript 5.5. Should we default it to ES2015? And if it's explicitly set to ES5, do we force it to be ES2015?

Let's go with this: Default to Latest if nothing is set, like currently. If something earlier than ES2015 is set, set to ES2015 (added a commit with that logic)

[dummdidumm]

Another clarification question: You linked several issues above - which of those are actually fixed by this PR?

• Project References support #2148 - I guess this one is solved
• svelte-check ignoring jsconfig.json's exclude option #1234 - it sounds like this is also solved then? Though your comment in there is somewhat valid: is there any way how we can ease the migration a bit for people having outdated tsconfigs that don't have Svelte files in their includes? Update: Looked around a bit how includes look - is this really a widespread problem? If I understand correclty src/**/* would also work, so the only way this can fail is if you only have something like src/**/*.ts in your includes. If no includes is set whatsoever, TS will include everything it finds, and that hopefully also includes Svelte files?
• Svelte language server forces too many tsconfig options #1976 - I guess this one is solved, too

[jasonlyu123]

Yeah. These 3 are fixed by this PR. I am a bit uncertain if these should be separated but turns out to be easier to do it at once.

is there any way how we can ease the migration a bit

What about we do something like that No inputs were found in config file '{0}'. Specified 'include' paths were '{1}' and 'exclude' paths were '{2}'. diagnostic but check only svelte files? We still create the service based on the nearest tsconfig.json and use that to check the project files. So if the service is created and parsedCommandLine.fileNames doesn't have any svelte file, it doesn't have references and files (https://github.com/microsoft/TypeScript/blob/c8a7d589e647e19c94150d9892909f3aa93e48eb/src/compiler/commandLineParser.ts#L3216) we'll report a similar diagnostic on the tsconfig.

[dummdidumm]

To reiterate: If

• we open a Svelte file
• we find a nearby tsconfig
• it doesn't have project references
• we read the file and it turns out there is no Svelte file at all in there
• THEN we issue a warning (or error?) (but where?) (if it's a warning do we fall back to something?) ?

sounds good to me. For svelte-check we can error, for VS Code I'm not really sure what to do. If we error for svelte-check only, that likely means you need to read the returned diagnostics property of the "parse the config" method, right? In that case, maybe we can combine that with #2154

[jasonlyu123]

I set the diagnostics in the config file.

And for the svelte-check, I put it in the configErrors. we can probably change it to show every configErrors in svelte-check for #2154. But since there are more errors probably need to print it like other diagnostics and need adjustment with the writer. We probably don't need to show the code for config errors. we previously didn't just throw an error and tsc also doesn't.