-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
feat: Project reference support #2463
Conversation
devDependencies pinned to Svelte 3 because other packages in this repo still use it. Theoretically we still support Svelte 3 with svelte-check v4 but this gives us the opportunity to adjust that later without a major
…nto project-reference
the fix for diagnostic made the flaky test appears a lot more often
…e while scheduling project file update
since it can run before or after project-files updates so either file might open a wrong service
… access the lsContainer
Nice work!
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?
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.
Looking at the changed code there isn't any change to the tsconfig options? Did you revert that to make it separate?
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? |
Take our repo for example. The root tsconfig is a "solution style" tsconfig which includes nothing and is meant to load
More like "a nice side effect". We'll need the compiler host created by TypeScript to redirect project reference
If there is tsconfig the forced option won't override the tsconfig. The
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( |
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?
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?
What about only setting target if we see it's either a very old one that likely breaks, or is not set at all? |
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
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 😅.
The default is ES5 in typescript 5.5. Should we default it to |
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) |
Another clarification question: You linked several issues above - which of those are actually fixed by this PR?
|
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.
What about we do something like that |
To reiterate: If
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 |
oops some stuff actually don't work in directory with uppercase
I set the diagnostics in the config file. And for the svelte-check, I put it in the |
Only editor shows the config error of referenced project so don't push it to the configErrors
Svelte files are now resolved by intercepting checks for .d.svelte.ts instead of .svelte.ts. As a consequence, we can handle sibling foo.svelte / foo.svelte.ts files and importing both. When doing import Foo from './foo.svelte this now resolves to the Svelte file (prio to #2463 it would resovle to the .svelte.ts file), and doing import Foo from './foo.svelte.js will resolve to the TS file.
#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:
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
?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.
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 thetarget
default breaks a lot of our tests and might confuse a lot of users.if an open file is not a project file for any tsconfig, it'll be treated as if there isn't a tsconfig.
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 botholdPath
andnewPath
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.