-
Notifications
You must be signed in to change notification settings - Fork 41.2k
npm: Use fs API to read package.json, avoid LSP notification spam #292380
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
Changes from 4 commits
0360cef
9325890
a2c32ad
849be58
2a2cf9a
fc51b35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import * as fs from 'fs'; | |
| import minimatch from 'minimatch'; | ||
| import { Utils } from 'vscode-uri'; | ||
| import { findPreferredPM } from './preferred-pm'; | ||
| import { readScripts } from './readScripts'; | ||
| import { readScripts, IPositionOffsetTransformer } from './readScripts'; | ||
|
|
||
| const excludeRegex = new RegExp('^(node_modules|.vscode-test)$', 'i'); | ||
|
|
||
|
|
@@ -31,6 +31,14 @@ type AutoDetect = 'on' | 'off'; | |
|
|
||
| let cachedTasks: ITaskWithLocation[] | undefined = undefined; | ||
|
|
||
| interface IPackageJsonCache { | ||
| mtime: number; | ||
| scripts: ReturnType<typeof readScripts>; | ||
| } | ||
|
|
||
| // Cache for package.json file reading to avoid unnecessary reads | ||
| const packageJsonCache = new Map<string, IPackageJsonCache>(); | ||
|
|
||
| export const INSTALL_SCRIPT = 'install'; | ||
|
|
||
| export interface ITaskLocation { | ||
|
|
@@ -88,6 +96,7 @@ export class NpmTaskProvider implements TaskProvider { | |
|
|
||
| export function invalidateTasksCache() { | ||
| cachedTasks = undefined; | ||
| packageJsonCache.clear(); | ||
| } | ||
|
|
||
| const buildNames: string[] = ['build', 'compile', 'watch']; | ||
|
|
@@ -473,6 +482,39 @@ export function findScriptAtPosition(document: TextDocument, buffer: string, pos | |
| return undefined; | ||
| } | ||
|
|
||
| // Helper to transform offsets to positions for package.json content | ||
| class PositionOffsetTransformer implements IPositionOffsetTransformer { | ||
| private readonly lineOffsets: number[]; | ||
|
|
||
| constructor(public readonly uri: Uri, content: string) { | ||
| const lines = content.split(/\r?\n/); | ||
| this.lineOffsets = [0]; | ||
| for (let i = 0; i < lines.length; i++) { | ||
| const currentOffset = this.lineOffsets[i] + lines[i].length; | ||
| // Check if there's a \r character (Windows line ending) | ||
| const nextCharOffset = currentOffset < content.length ? content[currentOffset] : undefined; | ||
| const lineEndingLength = nextCharOffset === '\r' ? 2 : 1; | ||
| this.lineOffsets.push(currentOffset + lineEndingLength); | ||
| } | ||
| } | ||
|
|
||
| positionAt(offset: number): Position { | ||
| let low = 0; | ||
| let high = this.lineOffsets.length - 1; | ||
| while (low < high) { | ||
| const mid = Math.floor((low + high) / 2); | ||
| if (this.lineOffsets[mid] > offset) { | ||
| high = mid; | ||
| } else { | ||
| low = mid + 1; | ||
| } | ||
| } | ||
| const line = low - 1; | ||
| const character = offset - this.lineOffsets[line]; | ||
| return new Position(line, character); | ||
| } | ||
| } | ||
|
|
||
| export async function getScripts(packageJsonUri: Uri) { | ||
| if (packageJsonUri.scheme !== 'file') { | ||
| return undefined; | ||
|
|
@@ -484,8 +526,25 @@ export async function getScripts(packageJsonUri: Uri) { | |
| } | ||
|
|
||
| try { | ||
| const document: TextDocument = await workspace.openTextDocument(packageJsonUri); | ||
| return readScripts(document); | ||
| // Check cache with mtime to avoid unnecessary file reads | ||
| const stat = await workspace.fs.stat(packageJsonUri); | ||
| const cached = packageJsonCache.get(packageJson); | ||
|
|
||
| if (cached && cached.mtime === stat.mtime) { | ||
| return cached.scripts; | ||
| } | ||
|
|
||
| // Use workspace.fs.readFile instead of openTextDocument to avoid triggering | ||
| // textDocument/didOpen notifications to language servers | ||
| const content = await workspace.fs.readFile(packageJsonUri); | ||
| const text = Buffer.from(content).toString('utf8'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use TextDecoder here for portability @copilot
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in commit fc51b35. Replaced |
||
| const transformer = new PositionOffsetTransformer(packageJsonUri, text); | ||
| const scripts = readScripts(transformer, text); | ||
|
|
||
| // Cache the result | ||
| packageJsonCache.set(packageJson, { mtime: stat.mtime, scripts }); | ||
|
|
||
| return scripts; | ||
| } catch (e) { | ||
| const localizedParseError = l10n.t("Npm task detection: failed to parse the file {0}", packageJsonUri.fsPath); | ||
| throw new Error(localizedParseError); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there. Now a few more things:
vscode.ExtensionContext(passed to the activate() method)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 2a2cf9a. Implemented
PackageJsonLRUCacheclass with:activate()with theExtensionContext