(fix) only allow client files and project files to be root #2146

jasonlyu123 · 2023-08-31T09:02:26Z

This PR moved the tracking of "document is open by the client" to document so it can be copied over to ts snapshot and create a new documentManager.openClientDocument to replace openDoucment in existing places. This will mark the file as client files in one method call, so we don't have to add markAsOpenByClient to all existing tests. The reason that we need client files is that files might be opened by the client and not in the project files. One reason is that the user has an empty include in the tsconfig or has the default one. Also, we didn't watch Svelte files with File Watcher. So the new svelte file won't be in the project files. We probably should do it for something like git checkout, but that can be a separate PR.

Alternatively, we could also create a getClientSnapshot method to replace the current LanguageServiceContainer.getSnaphsot method in getLSAndTSDoc . This should mark any document that is passed in as a client file. But I haven't checked if this would cause the dep/index.ts in #2132 to be marked as a client file.

I have to use a TypeScript internal API and add a layer of synchronization because of a module resolution bug. The way getSnapshot prevents the problem before the amount of root files have changed so that TypeScript will check more thoroughly. The existing test is different from how it'll work in typical cases. It probably worked when it was introduced, but we later changed to not run getSnapshot in new ts/js files, so it's not actually preventing the error.

…nto root-files

not sure if it make a huge difference

some tests became flaky because syncing is too eager

jasonlyu123 · 2023-09-11T06:04:00Z

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

+     * Retrieves the LS for operations that don't need cross-files information.
+     * can save some time by not synchronizing languageService program
+     */
+    async getLsForSyntheticOperations(document: Document): Promise<{


I created this as a separate method because it gave it a more meaningful name, so the code intention is clearer. And, well, tsserver also does that😆,

jasonlyu123 · 2023-09-11T06:06:50Z

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

+            lang: langForSyntheticOperations,
+            tsDoc,
+            userPreferences
+        } = await this.lsAndTsDocResolver.getLsForSyntheticOperations(document);


Reuse completion check and JSDoc template both don't need the language service program to be up to date.

jasonlyu123 · 2023-09-11T06:10:04Z

...ges/language-server/test/plugins/typescript/features/FindComponentReferencesProvider.test.ts

@@ -8,16 +8,16 @@ import { LSAndTSDocResolver } from '../../../../src/plugins/typescript/LSAndTSDo
 import { pathToUrl } from '../../../../src/utils';
 import { serviceWarmup } from '../test-utils';

-const testDir = path.join(__dirname, '..');
+const testDir = path.join(__dirname, '..', 'testfiles');


This warms up the wrong directory. I fixed it while debugging why the test is flaky again.

jasonlyu123 · 2023-09-11T06:17:53Z

packages/language-server/test/plugins/typescript/features/DiagnosticsProvider.test.ts

@@ -43,7 +43,7 @@ describe('DiagnosticsProvider', function () {
        const newFilePath = normalizePath(path.join(testDir, 'doesntexistyet.js')) || '';
        writeFileSync(newFilePath, 'export default function foo() {}');
        assert.ok(existsSync(newFilePath));
-        await lsAndTsDocResolver.getSnapshot(newFilePath);
+        await lsAndTsDocResolver.invalidateModuleCache(newFilePath);


This test is the one that failed because getSnapshot no longer adds new root files, and it's the reason why I use the internal API hasInvalidatedResolutions.

jasonlyu123 · 2023-09-11T06:22:16Z

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

+        }
+
+        languageService.getProgram();
+        svelteModuleLoader.clearPendingInvalidations();


This wrapper layer is needed because we need to clear the invalidated files so they won't affect every program update check afterwards. And the hoops you have to jump through are probably why hasInvalidatedResolutions is a TypeScript internal API 😅.

dummdidumm

Damn, impressive work!

To summarize in my own words, to double-check I understood this all correctly:

we're shifting openedByClient elsewhere so that we no longer mark all TS/JS files we pick up along the way as open automatically, because that's wrong
we're now using openedByClient to filter the files, which is why we need to be more granular about what is marked as opened in the client
because we're no longer marking every TS/JS file as opened, we have to do some more work in other places to correctly clear the module resolution cache in situations like creation/deletion/move, which is what we need the failed lookup info etc for
because we have this additional logic now, we can also be more precise about when to invalidate the program (updateIfDirty)
as a drive-by, we now optimize a bit with the "get language service but don't sync" bit

My guess is this PR might make things a bit more performant in some cases?

dummdidumm · 2023-09-11T08:17:26Z

packages/language-server/src/plugins/typescript/module-loader.ts

@@ -49,6 +50,7 @@ class ModuleResolutionCache {
        this.cache.forEach((val, key) => {
            if (val && this.getCanonicalFileName(val.resolvedFileName) === resolvedModuleName) {
                this.cache.delete(key);
+                this.pendingInvalidations.add(key.split(':::').shift() || '');


Probably makes sense to have ::: as const at the top and use that at this point

dummdidumm · 2023-09-11T08:18:00Z

packages/language-server/src/plugins/typescript/module-loader.ts

@@ -61,16 +63,27 @@ class ModuleResolutionCache {
        const fileNameWithoutEnding =
            getLastPartOfPath(this.getCanonicalFileName(path)).split('.').shift() || '';
        this.cache.forEach((val, key) => {
+            const split = key.split(':::');


moduleName and containingFile retrieval can probably be combined, right now they both split.

jasonlyu123 · 2023-09-11T09:10:06Z

We have to do some more work in other places to correctly clear the module resolution cache in situations like creation/deletion/move

Yeah. But it's also another bug because the previous method doesn't work for ts/js file creation, probably because we changed to not load new files unless it's already loaded.

as a drive-by, we now optimize a bit with the "get language service but don't sync" bit

Nope. this is added because updateIfDirty might have called at the wrong time and might have prevented previous optimizations from working. The reason why we need to manually trigger updates is because TypeScript will automatically check it in most language features. And that internal API is called when TS is checking if they need to rebuild. If we manually trigger an update, we can then clear the invalidation as done.

Due to a change in svelte-check 3.5.2 (likely sveltejs/language-tools#2146), we can no longer have composite enabled, as svelte-check spits out errors about files in other project references not being listed in included files. This should hopefully not make a difference to us, as we weren't taking advantage of TypeScript's project compilation, as svelte-check doesn't support it, and we use esbuild for JS conversion.

jasonlyu123 added 8 commits August 26, 2023 16:04

only allow client files and project files to be root

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

4a98c91

implement internal api for invalidating

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

44dc09a

Merge branch 'master' of https://github.com/sveltejs/language-tools i…

9d20957

…nto root-files

fix test and some module cache invalidation issues

742fec2

cleanup, remove setTimeout updates for now

c5f560e

not sure if it make a huge difference

delay syncing program until it's needed

bc4b42c

some tests became flaky because syncing is too eager

add comment about why we need the internal API

2195d1e

more cleanup

c1ccdf3

jasonlyu123 force-pushed the root-files branch from f4e7f3a to c1ccdf3 Compare September 11, 2023 06:15

jasonlyu123 commented Sep 11, 2023

View reviewed changes

jasonlyu123 marked this pull request as ready for review September 11, 2023 06:23

dummdidumm reviewed Sep 11, 2023

View reviewed changes

feedback

12d1223

dummdidumm merged commit c8ae56e into sveltejs:master Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) only allow client files and project files to be root #2146

(fix) only allow client files and project files to be root #2146

jasonlyu123 commented Aug 31, 2023 •

edited

Loading

jasonlyu123 Sep 11, 2023

jasonlyu123 Sep 11, 2023

jasonlyu123 Sep 11, 2023

jasonlyu123 Sep 11, 2023

jasonlyu123 Sep 11, 2023

dummdidumm left a comment

dummdidumm Sep 11, 2023

dummdidumm Sep 11, 2023

jasonlyu123 commented Sep 11, 2023 •

edited

Loading

(fix) only allow client files and project files to be root #2146

(fix) only allow client files and project files to be root #2146

Conversation

jasonlyu123 commented Aug 31, 2023 • edited Loading

jasonlyu123 Sep 11, 2023

Choose a reason for hiding this comment

jasonlyu123 Sep 11, 2023

Choose a reason for hiding this comment

jasonlyu123 Sep 11, 2023

Choose a reason for hiding this comment

jasonlyu123 Sep 11, 2023

Choose a reason for hiding this comment

jasonlyu123 Sep 11, 2023

Choose a reason for hiding this comment

dummdidumm left a comment

Choose a reason for hiding this comment

dummdidumm Sep 11, 2023

Choose a reason for hiding this comment

dummdidumm Sep 11, 2023

Choose a reason for hiding this comment

jasonlyu123 commented Sep 11, 2023 • edited Loading

jasonlyu123 commented Aug 31, 2023 •

edited

Loading

jasonlyu123 commented Sep 11, 2023 •

edited

Loading