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: only trigger validations for persisted files #186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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 @@ -11,9 +11,7 @@ const OPTIMIZED_PATHS_TO_FUNC: Map<
> = new Map();

// https://ux.stackexchange.com/questions/95336/how-long-should-the-debounce-timeout-be
// If the debounce is too small, changes may not be persisted in time to the disk for our
// the validation logic to run correctly.
const DEBOUNCE_WAIT = 1500;
const DEBOUNCE_WAIT = 500;

export function getOptimizedRefreshDiagnostics(
uri: Uri
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const pathToVersion: Map<string, number> = new Map();

/**
* Helper to identify when a newly persisted document in vscode represents
* the latest version.
*
* This is needed to help identify which of the **four** events `onDidChangeTextDocument`
* which happen on every key click is the "real" trigger for updating the diagnostics
*/
export function isNewPersistedFileVersion(
path: string,
newVer: number
): boolean {
if (pathToVersion.has(path)) {
const oldVer = pathToVersion.get(path)!;
if (newVer > oldVer) {
pathToVersion.set(path, newVer);
return true;
} else {
return false;
}
} else {
pathToVersion.set(path, newVer);
}

return false;
}
22 changes: 12 additions & 10 deletions packages/vscode-dependencies-validation/src/editorChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,11 @@ import type {
TextDocumentChangeEvent,
TextEditor,
} from "vscode";
import { isEmpty } from "lodash";
import { clearDiagnostics } from "./util";
import { VscodePackageJsonChangesConfig } from "./vscodeTypes";
import { getOptimizedRefreshDiagnostics } from "./diagnostics/debounce";
import { shouldBeChecked } from "./diagnostics/shouldBeChecked";

// TODO: when node_modules deleted or changed diagnostics are not refreshed and errors are not shown for an opened package.json
// the package.json file should be closed and opened again to trigger diagnostic refresh
import { isNewPersistedFileVersion } from "./diagnostics/isNewPersistedFileVersion";

export function subscribeToPackageJsonChanges(
vscodeConfig: VscodePackageJsonChangesConfig
Expand Down Expand Up @@ -50,7 +47,11 @@ function executeRefreshDiagnosticsOnEditorChange(
diagnosticCollection: DiagnosticCollection
) {
return (editor: TextEditor | undefined) => {
if (editor && shouldBeChecked(editor.document.uri.path)) {
if (
editor &&
shouldBeChecked(editor.document.uri.path) &&
!editor.document.isDirty
) {
{
const uri = editor.document.uri;
const optimizedRefreshDiag = getOptimizedRefreshDiagnostics(uri);
Expand All @@ -64,13 +65,14 @@ function executeRefreshDiagnosticsOnDocumentChangeEvent(
diagnosticCollection: DiagnosticCollection
) {
return (event: TextDocumentChangeEvent) => {
const uri = event.document.uri;
const document = event.document;
const uri = document.uri;

if (
// this event gets called multiple (four) times for each text change
// but only once for "true" text changes
!isEmpty(event.contentChanges) &&
shouldBeChecked(uri.path)
shouldBeChecked(uri.path) &&
!document.isDirty &&
// this check should be last as it may modify global state (path -> version cache)
isNewPersistedFileVersion(uri.path, document.version)
) {
const optimizedRefreshDiag = getOptimizedRefreshDiagnostics(uri);
void optimizedRefreshDiag(uri, diagnosticCollection);
Expand Down