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

♻️ Use LSP based file watcher #1610

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
3 changes: 1 addition & 2 deletions docs/user/config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ There's no support for multiple config files or inheriting/overriding config fil
"language": "Default",
"permissionLevel": 2,
"plugins": [],
"mcmetaSummaryOverrides": {},
"useFilePolling": false
"mcmetaSummaryOverrides": {}
},
"format": {
"blockStateBracketSpacing": { "inside": 0 },
Expand Down
916 changes: 514 additions & 402 deletions package-lock.json

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
"docs:start": "cd docs && (bundle exec jekyll serve --incremental || true) && cd ..",
"fmt": "dprint fmt",
"fmt:check": "dprint check",
"lint": "eslint --cache --max-warnings=0 packages/*/{src,test}/**/*.{cts,mts,ts}",
"lint:fix": "eslint --cache --max-warnings=0 --fix packages/*/{src,test}/**/*.{cts,mts,ts}",
"lint": "eslint --cache --max-warnings=0 \"packages/*/{src,test}/**/*.{cts,mts,ts}\"",
"lint:fix": "eslint --cache --max-warnings=0 --fix \"packages/*/{src,test}/**/*.{cts,mts,ts}\"",
"release": "ts-node scripts/release.ts",
"release:dry": "ts-node scripts/release.ts --dry-run",
"set-tsconfig-references": "ts-node scripts/set_tsconfig_references",
Expand Down
1 change: 0 additions & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"dependencies": {
"base64-arraybuffer": "^1.0.2",
"binary-search": "^1.3.6",
"chokidar": "^3.5.2",
"decompress": "^4.2.1",
"follow-redirects": "^1.14.8",
"ignore": "^5.3.1",
Expand Down
21 changes: 0 additions & 21 deletions packages/core/src/common/externals/BrowserExternals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,24 +71,6 @@ class BrowserExternalDownloader implements ExternalDownloader {
}
}

class BrowserFsWatcher implements FsWatcher {
on(event: string, listener: (...args: any[]) => unknown): this {
if (event === 'ready') {
listener()
}
return this
}

once(event: unknown, listener: (...args: any[]) => unknown): this {
if (event === 'ready') {
listener()
}
return this
}

async close(): Promise<void> {}
}

class BrowserFileSystem implements ExternalFileSystem {
private static readonly LocalStorageKey = 'spyglassmc-browser-fs'
private states: Record<string, { type: 'file'; content: string } | { type: 'directory' }>
Expand Down Expand Up @@ -149,9 +131,6 @@ class BrowserFileSystem implements ExternalFileSystem {
delete this.states[location]
this.saveStates()
}
watch(_locations: FsLocation[]): FsWatcher {
return new BrowserFsWatcher()
}
async writeFile(
location: FsLocation,
data: string | Uint8Array,
Expand Down
30 changes: 1 addition & 29 deletions packages/core/src/common/externals/NodeJsExternals.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/60592
import chokidar from 'chokidar'
import decompress from 'decompress'
import followRedirects from 'follow-redirects'
import { Buffer } from 'node:buffer'
Expand All @@ -19,7 +18,7 @@ import type {
ExternalDownloaderOptions,
RemoteUriString,
} from './downloader.js'
import type { Externals, FsLocation, FsWatcher } from './index.js'
import type { Externals, FsLocation } from './index.js'

const { http, https } = followRedirects
const gunzip = promisify(zlib.gunzip)
Expand Down Expand Up @@ -114,14 +113,6 @@ export const NodeJsExternals: Externals = {
unlink(location) {
return fsp.unlink(toFsPathLike(location))
},
watch(locations, { usePolling = false } = {}) {
return new ChokidarWatcherWrapper(
chokidar.watch(locations.map(toPath), {
usePolling,
disableGlobbing: true,
}),
)
},
writeFile(location, data, options) {
return fsp.writeFile(toFsPathLike(location), data, options)
},
Expand Down Expand Up @@ -154,22 +145,3 @@ function toPath(path: FsLocation): string {

const uriToPath = (uri: string | Uri) =>
url.fileURLToPath(uri instanceof Uri ? new url.URL(uri.toString()) : uri)
const uriFromPath = (path: string) => url.pathToFileURL(path).toString()

class ChokidarWatcherWrapper extends EventEmitter implements FsWatcher {
readonly #watcher: chokidar.FSWatcher

constructor(watcher: chokidar.FSWatcher) {
super()
this.#watcher = watcher
.on('ready', () => this.emit('ready'))
.on('add', (path) => this.emit('add', uriFromPath(path)))
.on('change', (path) => this.emit('change', uriFromPath(path)))
.on('unlink', (path) => this.emit('unlink', uriFromPath(path)))
.on('error', (e) => this.emit('error', e))
}

close(): Promise<void> {
return this.#watcher.close()
}
}
8 changes: 7 additions & 1 deletion packages/core/src/common/externals/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export interface ExternalFileSystem {
showFile(path: FsLocation): Promise<void>
stat(location: FsLocation): Promise<{ isDirectory(): boolean; isFile(): boolean }>
unlink(location: FsLocation): Promise<void>
watch(locations: FsLocation[], options: { usePolling?: boolean }): FsWatcher
/**
* @param options `mode` - File mode bit mask (e.g. `0o775`).
*/
Expand All @@ -88,7 +87,14 @@ export interface ExternalFileSystem {
*/
export type FsLocation = string | Uri

/**
* A file system watcher that reports additions, changes, and deletions of files.
* Changes to directories should not be reported.
*/
export interface FsWatcher {
SPGoding marked this conversation as resolved.
Show resolved Hide resolved
get isReady(): boolean
get watchedFiles(): Set<string>

on(eventName: 'ready', listener: () => unknown): this
once(eventName: 'ready', listener: () => unknown): this

Expand Down
11 changes: 0 additions & 11 deletions packages/core/src/service/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,16 +115,6 @@ export interface EnvConfig {
>
permissionLevel: 1 | 2 | 3 | 4
plugins: string[]
/**
* Makes the file-watcher use polling to watch for file changes.
* Comes at a performance cost for very large datapacks.
*
* On Windows, enabling this can fix file-lock issues when Spyglass is running.
* See: https://github.com/SpyglassMC/Spyglass/issues/1414
*
* **You should only consider enabling this for Windows machines.**
*/
useFilePolling: boolean
}

export type LinterSeverity = 'hint' | 'information' | 'warning' | 'error'
Expand Down Expand Up @@ -362,7 +352,6 @@ export const VanillaConfig: Config = {
permissionLevel: 2,
plugins: [],
mcmetaSummaryOverrides: {},
useFilePolling: false,
},
format: {
blockStateBracketSpacing: { inside: 0 },
Expand Down
58 changes: 24 additions & 34 deletions packages/core/src/service/Project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export interface ProjectOptions {
* File URIs to the roots of this project.
*/
projectRoots: RootUriString[]
projectRootsWatcher?: FsWatcher
symbols?: SymbolUtil
}

Expand Down Expand Up @@ -173,9 +174,10 @@ export class Project implements ExternalEventEmitter {
readonly #initializers: readonly ProjectInitializer[]
#initPromise!: Promise<void>
#readyPromise!: Promise<void>
readonly #watchedFiles = new Set<string>()
#watcher!: FsWatcher
#watcherReady = false
readonly #watcher: FsWatcher | undefined
get watchedFiles() {
return this.#watcher?.watchedFiles ?? new Set()
}

#isReady = false
get isReady(): boolean {
Expand Down Expand Up @@ -291,7 +293,7 @@ export class Project implements ExternalEventEmitter {
*/
getTrackedFiles(): string[] {
const extensions: string[] = this.meta.getSupportedFileExtensions()
const supportedFiles = [...this.#dependencyFiles ?? [], ...this.#watchedFiles]
const supportedFiles = [...this.#dependencyFiles ?? [], ...this.watchedFiles]
.filter((file) => extensions.includes(fileUtil.extname(file) ?? ''))
const filteredFiles = this.ignore.filter(supportedFiles)
return filteredFiles
Expand All @@ -309,6 +311,7 @@ export class Project implements ExternalEventEmitter {
logger = Logger.create(),
profilers = ProfilerFactory.noop(),
projectRoots,
projectRootsWatcher,
}: ProjectOptions,
) {
this.#cacheRoot = cacheRoot
Expand All @@ -320,6 +323,7 @@ export class Project implements ExternalEventEmitter {
this.logger = logger
this.profilers = profilers
this.projectRoots = projectRoots
this.#watcher = projectRootsWatcher

this.cacheService = new CacheService(cacheRoot, this)
this.#configService = new ConfigService(this, defaultConfig)
Expand Down Expand Up @@ -510,34 +514,21 @@ export class Project implements ExternalEventEmitter {
}
const listProjectFiles = () =>
new Promise<void>((resolve) => {
if (this.projectRoots.length === 0) {
resolve()
return
if (!this.#watcher) {
return resolve()
}
this.#watchedFiles.clear()
this.#watcherReady = false
this.#watcher = this.externals.fs.watch(this.projectRoots, {
usePolling: this.config.env.useFilePolling,
}).once('ready', () => {
this.#watcherReady = true

this.#watcher
.on('add', (uri) => this.emit('fileCreated', { uri }))
.on('change', (uri) => this.emit('fileModified', { uri }))
.on('unlink', (uri) => this.emit('fileDeleted', { uri }))
.on('error', (e) => this.logger.error('[Project#watcher]', e))

if (this.#watcher.isReady) {
resolve()
}).on('add', (uri) => {
this.#watchedFiles.add(uri)
if (this.#watcherReady) {
this.emit('fileCreated', { uri })
}
}).on('change', (uri) => {
if (this.#watcherReady) {
this.emit('fileModified', { uri })
}
}).on('unlink', (uri) => {
this.#watchedFiles.delete(uri)
if (this.#watcherReady) {
this.emit('fileDeleted', { uri })
}
}).on('error', (e) => {
this.logger.error('[Project] [chokidar]', e)
})
} else {
this.#watcher.on('ready', resolve)
}
})
const ready = async () => {
await this.init()
Expand Down Expand Up @@ -621,13 +612,12 @@ export class Project implements ExternalEventEmitter {
*/
async close(): Promise<void> {
clearInterval(this.#cacheSaverIntervalId)
await this.#watcher.close()
await this.#watcher?.close()
await this.cacheService.save()
}

async restart(): Promise<void> {
try {
await this.#watcher.close()
this.#bindingInProgressUris.clear()
this.#symbolUpToDateUris.clear()
this.setReadyPromise()
Expand Down Expand Up @@ -979,11 +969,11 @@ export class Project implements ExternalEventEmitter {
private shouldRemove(uri: string): boolean {
return (!this.#clientManagedUris.has(uri)
&& !this.#dependencyFiles?.has(uri)
&& !this.#watchedFiles.has(uri))
&& !this.watchedFiles.has(uri))
}

private isOnlyWatched(uri: string): boolean {
return (this.#watchedFiles.has(uri)
return (this.watchedFiles.has(uri)
&& !this.#clientManagedUris.has(uri)
&& !this.#dependencyFiles?.has(uri))
}
Expand Down
32 changes: 29 additions & 3 deletions packages/language-server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
MyLspDataHackPubifyRequestParams,
} from './util/index.js'
import { toCore, toLS } from './util/index.js'
import { LspFsWatcher } from './util/LspFsWatcher.js'

export * from './util/types.js'

Expand All @@ -28,9 +29,9 @@ const { cache: cacheRoot } = envPaths('spyglassmc')

const connection = ls.createConnection()
let capabilities!: ls.ClientCapabilities
let fsWatcher: LspFsWatcher | undefined
let workspaceFolders!: ls.WorkspaceFolder[]
let hasShutdown = false
let progressReporter: ls.WorkDoneProgressReporter | undefined

const externals = NodeJsExternals
const logger: core.Logger = {
Expand All @@ -52,6 +53,7 @@ connection.onInitialize(async (params) => {

capabilities = params.capabilities
workspaceFolders = params.workspaceFolders ?? []
const projectRoots = workspaceFolders.map(f => core.fileUtil.ensureEndingSlash(f.uri))

if (initializationOptions?.inDevelopmentMode) {
await new Promise((resolve) => setTimeout(resolve, 3000))
Expand All @@ -66,6 +68,16 @@ connection.onInitialize(async (params) => {
logger.error('[loadLocale]', e)
}

if (capabilities.workspace?.didChangeWatchedFiles?.dynamicRegistration) {
// Initializes LspFsWatcher when client supports didChangeWatchedFiles notifications.
fsWatcher = new LspFsWatcher(projectRoots, logger)
.on('ready', () => logger.info('[FsWatcher] ready'))
.on('add', (uri) => logger.info('[FsWatcher] added', uri))
.on('change', (uri) => logger.info('[FsWatcher] changed', uri))
.on('unlink', (uri) => logger.info('[FsWatcher] unlinked', uri))
.on('error', (e) => logger.error('[FsWatcher]', e))
}

try {
service = new core.Service({
isDebugging: initializationOptions?.inDevelopmentMode,
Expand All @@ -84,7 +96,8 @@ connection.onInitialize(async (params) => {
cacheRoot: fileUtil.ensureEndingSlash(url.pathToFileURL(cacheRoot).toString()),
externals,
initializers: [mcdoc.initialize, je.initialize],
projectRoots: workspaceFolders.map(f => core.fileUtil.ensureEndingSlash(f.uri)),
projectRoots,
projectRootsWatcher: fsWatcher,
},
})
service.project.on('documentErrored', async ({ errors, uri, version }) => {
Expand Down Expand Up @@ -150,6 +163,16 @@ connection.onInitialize(async (params) => {
})

connection.onInitialized(async () => {
if (capabilities.workspace?.didChangeWatchedFiles?.dynamicRegistration) {
const watcherDisposable = await connection.client.register(
ls.DidChangeWatchedFilesNotification.type,
{
watchers: [{ globPattern: '**/*' }],
Copy link
Member

Choose a reason for hiding this comment

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

In #1607 Afro started working on being able to exclude certain patterns from the watcher. From testing, the watcher seems to be very performant so this may not be necessary, but I did see raw LSP changes in .git/FETCH_HEAD for example. Do these have potential to break anything? If we want to do any filtering, should we do it here or later in LspFsWatcher.onLspDidChangeWatchedFiles?

},
)
fsWatcher!.setLspListener(watcherDisposable)
}

await service.project.ready()
if (capabilities.workspace?.workspaceFolders) {
connection.workspace.onDidChangeWorkspaceFolders(async () => {
Expand All @@ -171,7 +194,10 @@ connection.onDidCloseTextDocument(({ textDocument: { uri } }) => {
service.project.onDidClose(uri)
})

connection.workspace.onDidRenameFiles(({}) => {})
connection.onDidChangeWatchedFiles((params) => {
logger.info('[FsWatcher] raw LSP changes', params)
void fsWatcher!.onLspDidChangeWatchedFiles(params)
})

connection.onColorPresentation(async ({ textDocument: { uri }, color, range }) => {
const docAndNode = await service.project.ensureClientManagedChecked(uri)
Expand Down
Loading
Loading