Skip to content
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

fix no-missing-import getting confused on incremental file changes #338

Merged
merged 1 commit into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between these methods?

Copy link
Contributor Author

@AndrewJakubowicz AndrewJakubowicz Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no difference except it looked like the intent was that importedComponentDefinitionsInFile should be private, with absorbComponentDefinitionsForFile being the public method to do this logic.

You can see the dependencyStore class defined here: https://github.com/runem/lit-analyzer/pull/338/files#diff-faaba94b3ffda920023b5e0ff3b54f7137199117142a418b7bd79df7e8951c5eR6-R9

This is a pure refactor, no logic change. I can revert this line if it keeps it simpler.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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())
);
}
Comment on lines +171 to +180
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue. The rest of the PR is refactor & tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genius. Nice find.

}

if (result?.resolvedModule?.resolvedFileName != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,3 @@
export interface AnalyzerDependencyStore {
hasTagNameBeenImported(fileName: string, tagName: string): boolean;
}

//importedComponentDefinitionsInFile = new Map<string, ComponentDefinition[]>();

/**
* 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;
}*/
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, ComponentDefinition[]>();
private importedComponentDefinitionsInFile = new Map<string, ComponentDefinition[]>();

absorbComponentDefinitionsForFile(sourceFile: SourceFile, result: ComponentDefinition[]): void {
this.importedComponentDefinitionsInFile.set(sourceFile.fileName, result);
Expand Down
Original file line number Diff line number Diff line change
@@ -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`<my-defined-element></my-defined-element><my-other-element></my-other-element>`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export class MyDefinedElement extends HTMLElement {}

customElements.define("my-defined-element", MyDefinedElement);

declare global {
interface HTMLElementTagNameMap {
"my-defined-element": MyDefinedElement;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export class MyOtherElement extends HTMLElement {}

customElements.define("my-other-element", MyOtherElement);

declare global {
interface HTMLElementTagNameMap {
"my-other-element": MyOtherElement;
}
}
58 changes: 43 additions & 15 deletions packages/vscode-lit-plugin/src/test/simple-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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!");
Expand All @@ -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 <my-other-element>\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);
Expand Down