From 9f44f6f3843328c5277edec695669f1d4dd57397 Mon Sep 17 00:00:00 2001 From: Andrew Jakubowicz Date: Fri, 5 Jan 2024 10:26:31 -0800 Subject: [PATCH] fix no-missing-import getting confused on incremental file changes (#338) --- .../analyze/default-lit-analyzer-context.ts | 2 +- .../parse-dependencies/visit-dependencies.ts | 10 ++++ .../store/analyzer-dependency-store.ts | 17 ------ .../default-analyzer-dependency-store.ts | 2 +- .../src/test/fixtures/missing-import.ts | 7 +++ .../src/test/fixtures/my-defined-element.ts | 9 +++ .../src/test/fixtures/my-other-element.ts | 9 +++ .../vscode-lit-plugin/src/test/simple-test.ts | 58 ++++++++++++++----- 8 files changed, 80 insertions(+), 34 deletions(-) create mode 100644 packages/vscode-lit-plugin/src/test/fixtures/missing-import.ts create mode 100644 packages/vscode-lit-plugin/src/test/fixtures/my-defined-element.ts create mode 100644 packages/vscode-lit-plugin/src/test/fixtures/my-other-element.ts diff --git a/packages/lit-analyzer/src/lib/analyze/default-lit-analyzer-context.ts b/packages/lit-analyzer/src/lib/analyze/default-lit-analyzer-context.ts index 50cb6a29..b918c851 100644 --- a/packages/lit-analyzer/src/lib/analyze/default-lit-analyzer-context.ts +++ b/packages/lit-analyzer/src/lib/analyze/default-lit-analyzer-context.ts @@ -285,6 +285,6 @@ export class DefaultLitAnalyzerContext implements LitAnalyzerContext { // Build a graph of component dependencies const res = parseDependencies(file, this); - this.dependencyStore.importedComponentDefinitionsInFile.set(file.fileName, res); + this.dependencyStore.absorbComponentDefinitionsForFile(file, res); } } diff --git a/packages/lit-analyzer/src/lib/analyze/parse/parse-dependencies/visit-dependencies.ts b/packages/lit-analyzer/src/lib/analyze/parse/parse-dependencies/visit-dependencies.ts index 350a4f97..9c0faa72 100644 --- a/packages/lit-analyzer/src/lib/analyze/parse/parse-dependencies/visit-dependencies.ts +++ b/packages/lit-analyzer/src/lib/analyze/parse/parse-dependencies/visit-dependencies.ts @@ -168,6 +168,16 @@ function emitDirectModuleImportWithName(moduleSpecifier: string, node: Node, con if (cache != null) { result = context.ts.resolveModuleNameFromCache(moduleSpecifier, node.getSourceFile().fileName, cache, mode); } + if (result == null) { + // Result could not be found from the cache, try and resolve module without using the + // cache. + result = context.ts.resolveModuleName( + moduleSpecifier, + node.getSourceFile().fileName, + context.program.getCompilerOptions(), + context.ts.createCompilerHost(context.program.getCompilerOptions()) + ); + } } if (result?.resolvedModule?.resolvedFileName != null) { diff --git a/packages/lit-analyzer/src/lib/analyze/store/analyzer-dependency-store.ts b/packages/lit-analyzer/src/lib/analyze/store/analyzer-dependency-store.ts index 0bb29920..03e7662f 100644 --- a/packages/lit-analyzer/src/lib/analyze/store/analyzer-dependency-store.ts +++ b/packages/lit-analyzer/src/lib/analyze/store/analyzer-dependency-store.ts @@ -1,20 +1,3 @@ export interface AnalyzerDependencyStore { hasTagNameBeenImported(fileName: string, tagName: string): boolean; } - -//importedComponentDefinitionsInFile = new Map(); - -/** - * Returns if a component for a specific file has been imported. - * @param fileName - * @param tagName - */ -/*hasTagNameBeenImported(fileName: string, tagName: string): boolean { - for (const file of this.importedComponentDefinitionsInFile.get(fileName) || []) { - if (file.tagName === tagName) { - return true; - } - } - - return false; - }*/ diff --git a/packages/lit-analyzer/src/lib/analyze/store/dependency-store/default-analyzer-dependency-store.ts b/packages/lit-analyzer/src/lib/analyze/store/dependency-store/default-analyzer-dependency-store.ts index c06c920c..ba2b37be 100644 --- a/packages/lit-analyzer/src/lib/analyze/store/dependency-store/default-analyzer-dependency-store.ts +++ b/packages/lit-analyzer/src/lib/analyze/store/dependency-store/default-analyzer-dependency-store.ts @@ -3,7 +3,7 @@ import { ComponentDefinition } from "web-component-analyzer"; import { AnalyzerDependencyStore } from "../analyzer-dependency-store.js"; export class DefaultAnalyzerDependencyStore implements AnalyzerDependencyStore { - importedComponentDefinitionsInFile = new Map(); + private importedComponentDefinitionsInFile = new Map(); absorbComponentDefinitionsForFile(sourceFile: SourceFile, result: ComponentDefinition[]): void { this.importedComponentDefinitionsInFile.set(sourceFile.fileName, result); diff --git a/packages/vscode-lit-plugin/src/test/fixtures/missing-import.ts b/packages/vscode-lit-plugin/src/test/fixtures/missing-import.ts new file mode 100644 index 00000000..ebbb061d --- /dev/null +++ b/packages/vscode-lit-plugin/src/test/fixtures/missing-import.ts @@ -0,0 +1,7 @@ +import "./my-defined-element.js"; + +// Pretending this is the Lit html function +// eslint-disable-next-line @typescript-eslint/no-explicit-any +declare const html: any; + +html``; diff --git a/packages/vscode-lit-plugin/src/test/fixtures/my-defined-element.ts b/packages/vscode-lit-plugin/src/test/fixtures/my-defined-element.ts new file mode 100644 index 00000000..04478533 --- /dev/null +++ b/packages/vscode-lit-plugin/src/test/fixtures/my-defined-element.ts @@ -0,0 +1,9 @@ +export class MyDefinedElement extends HTMLElement {} + +customElements.define("my-defined-element", MyDefinedElement); + +declare global { + interface HTMLElementTagNameMap { + "my-defined-element": MyDefinedElement; + } +} diff --git a/packages/vscode-lit-plugin/src/test/fixtures/my-other-element.ts b/packages/vscode-lit-plugin/src/test/fixtures/my-other-element.ts new file mode 100644 index 00000000..f6b3b301 --- /dev/null +++ b/packages/vscode-lit-plugin/src/test/fixtures/my-other-element.ts @@ -0,0 +1,9 @@ +export class MyOtherElement extends HTMLElement {} + +customElements.define("my-other-element", MyOtherElement); + +declare global { + interface HTMLElementTagNameMap { + "my-other-element": MyOtherElement; + } +} diff --git a/packages/vscode-lit-plugin/src/test/simple-test.ts b/packages/vscode-lit-plugin/src/test/simple-test.ts index 82341299..91477555 100644 --- a/packages/vscode-lit-plugin/src/test/simple-test.ts +++ b/packages/vscode-lit-plugin/src/test/simple-test.ts @@ -7,6 +7,20 @@ import * as path from "path"; import * as vscode from "vscode"; // import * as litPlugin from "../extension.js"; +// wait until the TS language server is ready and diagnostics are produced +async function getDiagnostics(docUri: vscode.Uri, retries = 1000) { + for (let i = 0; i < retries; i++) { + const diagnostics = vscode.languages.getDiagnostics(docUri); + if (diagnostics.length > 0) { + return diagnostics; + } + // Is there a better way to wait for the ts server to be ready? + // Maybe we can listen for the event that displays and hides the "initializing TS/JS language features" message? + await new Promise(resolve => setTimeout(resolve, 100)); + } + throw new Error("No diagnostics found"); +} + suite("Extension Test Suite", () => { after(() => { vscode.window.showInformationMessage("All tests done!"); @@ -25,27 +39,41 @@ suite("Extension Test Suite", () => { const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(path.join(__dirname, "../../src/test/fixtures/missing-elem-type.ts"))); await vscode.window.showTextDocument(doc); - // wait until the TS language server is ready and diagnostics are produced - async function getDiagnostics() { - for (let i = 0; i < 1000; i++) { - const diagnostics = vscode.languages.getDiagnostics(doc.uri); - if (diagnostics.length > 0) { - return diagnostics; - } - // Is there a better way to wait for the ts server to be ready? - // Maybe we can listen for the event that displays and hides the "initializing TS/JS language features" message? - await new Promise(resolve => setTimeout(resolve, 100)); - } - throw new Error("No diagnostics found"); - } - - const diagnostics = await getDiagnostics(); + const diagnostics = await getDiagnostics(doc.uri); assert.deepStrictEqual( diagnostics.map(d => d.message), ["'my-element' has not been registered on HTMLElementTagNameMap"] ); }); + test("We detect no-missing-import properly", async () => { + const config = vscode.workspace.getConfiguration(); + config.update("lit-plugin.logging", "verbose", true); + config.update("lit-plugin.rules.no-missing-import", "error", true); + const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(path.join(__dirname, "../../src/test/fixtures/missing-import.ts"))); + const editor = await vscode.window.showTextDocument(doc); + + const diagnostics = await getDiagnostics(doc.uri); + assert.deepStrictEqual( + diagnostics.map(d => d.message), + ["Missing import for \n You can disable this check by disabling the 'no-missing-import' rule."] + ); + + // now add the fix + const editRange = doc.lineAt(0).range.start; + editor.edit(builder => { + if (!editor) { + throw new Error("No editor found"); + } + editor.insertSnippet(new vscode.SnippetString("import './my-other-element';\n"), editRange); + }); + + // give it some time to settle + await new Promise(resolve => setTimeout(resolve, 1000)); + + assert.rejects(getDiagnostics(doc.uri, 3), "Expected rejection as no diagnostics will be found."); + }); + test("We generate completions", async () => { const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(path.join(__dirname, "../../src/test/fixtures/completions.ts"))); const editor = await vscode.window.showTextDocument(doc);