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

chore: switch from fast-glob to fdir #2433

Merged
merged 5 commits into from
Aug 19, 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
2 changes: 1 addition & 1 deletion packages/language-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"@vscode/emmet-helper": "2.8.4",
"chokidar": "^3.4.1",
"estree-walker": "^2.0.1",
"fast-glob": "^3.2.7",
"fdir": "^6.2.0",
"lodash": "^4.17.21",
"prettier": "~3.2.5",
"prettier-plugin-svelte": "^3.2.2",
Expand Down
27 changes: 18 additions & 9 deletions packages/language-server/src/lib/documents/configLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CompileOptions } from 'svelte/types/compiler/interfaces';
// @ts-ignore
import { PreprocessorGroup } from 'svelte/types/compiler/preprocess';
import { importSveltePreprocess } from '../../importPackage';
import _glob from 'fast-glob';
import { fdir } from 'fdir';
import _path from 'path';
import _fs from 'fs';
import { pathToFileURL, URL } from 'url';
Expand Down Expand Up @@ -46,6 +46,8 @@ const _dynamicImport = new Function('modulePath', 'return import(modulePath)') a
modulePath: URL
) => Promise<any>;

const configRegex = /\/svelte\.config\.(js|cjs|mjs)$/;

/**
* Loads svelte.config.{js,cjs,mjs} files. Provides both a synchronous and asynchronous
* interface to get a config file because snapshots need access to it synchronously.
Expand All @@ -60,7 +62,7 @@ export class ConfigLoader {
private disabled = false;

constructor(
private globSync: typeof _glob.sync,
private globSync: typeof fdir,
private fs: Pick<typeof _fs, 'existsSync'>,
private path: Pick<typeof _path, 'dirname' | 'relative' | 'join'>,
private dynamicImport: typeof _dynamicImport
Expand All @@ -83,12 +85,19 @@ export class ConfigLoader {
Logger.log('Trying to load configs for', directory);

try {
const pathResults = this.globSync('**/svelte.config.{js,cjs,mjs}', {
cwd: directory,
// the second pattern is necessary because else fast-glob treats .tmp/../node_modules/.. as a valid match for some reason
ignore: ['**/node_modules/**', '**/.*/**'],
onlyFiles: true
});
const pathResults = new this.globSync({})
.withPathSeparator('/')
.exclude((_, path) => {
// no / at the start, path could start with node_modules
return path.includes('node_modules/') || path.includes('/.');
Copy link
Member

@brunnerh brunnerh Aug 27, 2024

Choose a reason for hiding this comment

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

Was the second term necessary?
According to the previous code, the pattern was added as a workaround to exclude node_modules in a . directory.

Copy link
Member

@dummdidumm dummdidumm Aug 27, 2024

Choose a reason for hiding this comment

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

It's necessary to not traverse into directories such as .git etc (directories starting with a dot are generally considered to be private), and I can't think of a situation where you would have a svelte.config.js in one of those directories - do you have such a case or why do you ask?

Copy link
Member

Choose a reason for hiding this comment

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

I have used . directories for caching/generated content but don't think they needed checking/processing so far.
Was just wondering if this could be simplified, but it's probably also faster this way.

})
.filter((path, isDir) => {
return !isDir && configRegex.test(path);
})
.withRelativePaths()
.crawl(directory)
.sync();

const someConfigIsImmediateFileInDirectory =
pathResults.length > 0 && pathResults.some((res) => !this.path.dirname(res));
if (!someConfigIsImmediateFileInDirectory) {
Expand Down Expand Up @@ -270,4 +279,4 @@ export class ConfigLoader {
}
}

export const configLoader = new ConfigLoader(_glob.sync, _fs, _path, _dynamicImport);
export const configLoader = new ConfigLoader(fdir, _fs, _path, _dynamicImport);
44 changes: 32 additions & 12 deletions packages/language-server/test/lib/documents/configLoader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,29 @@ describe('ConfigLoader', () => {
return path.join(...filePath.split('/'));
}

function mockFdir(results: string[] | (() => string[])): any {
return class {
withPathSeparator() {
return this;
}
exclude() {
return this;
}
filter() {
return this;
}
withRelativePaths() {
return this;
}
crawl() {
return this;
}
sync() {
return typeof results === 'function' ? results() : results;
}
};
}

async function assertFindsConfig(
configLoader: ConfigLoader,
filePath: string,
Expand All @@ -32,7 +55,7 @@ describe('ConfigLoader', () => {

it('should load all config files below and the one inside/above given directory', async () => {
const configLoader = new ConfigLoader(
(() => ['svelte.config.js', 'below/svelte.config.js']) as any,
mockFdir(['svelte.config.js', 'below/svelte.config.js']),
{ existsSync: () => true },
path,
(module: URL) => Promise.resolve({ default: { preprocess: module.toString() } })
Expand Down Expand Up @@ -63,7 +86,7 @@ describe('ConfigLoader', () => {

it('finds first above if none found inside/below directory', async () => {
const configLoader = new ConfigLoader(
() => [],
mockFdir([]),
{
existsSync: (p) =>
typeof p === 'string' && p.endsWith(path.join('some', 'svelte.config.js'))
Expand All @@ -78,7 +101,7 @@ describe('ConfigLoader', () => {

it('adds fallback if no config found', async () => {
const configLoader = new ConfigLoader(
() => [],
mockFdir([]),
{ existsSync: () => false },
path,
(module: URL) => Promise.resolve({ default: { preprocess: module.toString() } })
Expand All @@ -99,14 +122,14 @@ describe('ConfigLoader', () => {
let firstGlobCall = true;
let nrImportCalls = 0;
const configLoader = new ConfigLoader(
(() => {
mockFdir(() => {
if (firstGlobCall) {
firstGlobCall = false;
return ['svelte.config.js'];
} else {
return [];
}
}) as any,
}),
{
existsSync: (p) =>
typeof p === 'string' &&
Expand Down Expand Up @@ -140,11 +163,8 @@ describe('ConfigLoader', () => {
});

it('can deal with missing config', () => {
const configLoader = new ConfigLoader(
() => [],
{ existsSync: () => false },
path,
() => Promise.resolve('unimportant')
const configLoader = new ConfigLoader(mockFdir([]), { existsSync: () => false }, path, () =>
Promise.resolve('unimportant')
);
assert.deepStrictEqual(
configLoader.getConfig(normalizePath('/some/file.svelte')),
Expand All @@ -154,7 +174,7 @@ describe('ConfigLoader', () => {

it('should await config', async () => {
const configLoader = new ConfigLoader(
() => [],
mockFdir([]),
{ existsSync: () => true },
path,
(module: URL) => Promise.resolve({ default: { preprocess: module.toString() } })
Expand All @@ -168,7 +188,7 @@ describe('ConfigLoader', () => {
it('should not load config when disabled', async () => {
const moduleLoader = spy();
const configLoader = new ConfigLoader(
() => [],
mockFdir([]),
{ existsSync: () => true },
path,
moduleLoader
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte-check/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"dependencies": {
"@jridgewell/trace-mapping": "^0.3.17",
"chokidar": "^3.4.1",
"fdir": "^6.2.0",
"picocolors": "^1.0.0",
"sade": "^1.7.4",
"svelte-preprocess": "^5.1.3",
Expand All @@ -46,7 +47,6 @@
"@rollup/plugin-typescript": "^10.0.0",
"@types/sade": "^1.7.2",
"builtin-modules": "^3.3.0",
"fast-glob": "^3.2.7",
"rollup": "3.7.5",
"rollup-plugin-cleanup": "^3.2.0",
"rollup-plugin-copy": "^3.4.0",
Expand Down
47 changes: 42 additions & 5 deletions packages/svelte-check/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import { watch } from 'chokidar';
import * as fs from 'fs';
import glob from 'fast-glob';
import { fdir } from 'fdir';
import * as path from 'path';
import { SvelteCheck, SvelteCheckOptions } from 'svelte-language-server';
import { Diagnostic, DiagnosticSeverity } from 'vscode-languageserver-protocol';
Expand All @@ -30,11 +30,48 @@ async function openAllDocuments(
filePathsToIgnore: string[],
svelteCheck: SvelteCheck
) {
const files = await glob('**/*.svelte', {
cwd: workspaceUri.fsPath,
ignore: ['node_modules/**'].concat(filePathsToIgnore.map((ignore) => `${ignore}/**`))
const offset = workspaceUri.fsPath.length + 1;
// We support a very limited subset of glob patterns: You can only have ** at the end or the start
const ignored: Array<(path: string) => boolean> = filePathsToIgnore.map((i) => {
if (i.endsWith('**')) i = i.slice(0, -2);

if (i.startsWith('**')) {
i = i.slice(2);

if (i.includes('*'))
throw new Error(
'Invalid svelte-check --ignore pattern: Only ** at the start or end is supported'
);

return (path) => path.includes(i);
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
}

if (i.includes('*'))
throw new Error(
'Invalid svelte-check --ignore pattern: Only ** at the start or end is supported'
);

return (path) => path.startsWith(i);
});
const absFilePaths = files.map((f) => path.resolve(workspaceUri.fsPath, f));
const isIgnored = (path: string) => {
path = path.slice(offset);
for (const i of ignored) {
if (i(path)) {
return true;
}
}
return false;
};
const absFilePaths = await new fdir()
.filter((path) => path.endsWith('.svelte') && !isIgnored(path))
.exclude((_, path) => {
path = path.slice(offset);
return path.startsWith('.') || path.startsWith('node_modules');
})
.withPathSeparator('/')
.withFullPaths()
.crawl(workspaceUri.fsPath)
.withPromise();

for (const absFilePath of absFilePaths) {
const text = fs.readFileSync(absFilePath, 'utf-8');
Expand Down
6 changes: 1 addition & 5 deletions packages/svelte-check/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export function parseOptions(cb: (opts: SvelteCheckCliOptions) => any) {
watch: !!opts.watch,
preserveWatchOutput: !!opts.preserveWatchOutput,
tsconfig: getTsconfig(opts, workspaceUri.fsPath),
filePathsToIgnore: getFilepathsToIgnore(opts),
filePathsToIgnore: opts.ignore?.split(',') || [],
failOnWarnings: !!opts['fail-on-warnings'],
compilerWarnings: getCompilerWarnings(opts),
diagnosticSources: getDiagnosticSources(opts),
Expand Down Expand Up @@ -180,10 +180,6 @@ function getDiagnosticSources(opts: Record<string, any>): DiagnosticSource[] {
: diagnosticSources;
}

function getFilepathsToIgnore(opts: Record<string, any>): string[] {
return opts.ignore?.split(',') || [];
}

const thresholds = ['warning', 'error'] as const;
type Threshold = (typeof thresholds)[number];

Expand Down
22 changes: 16 additions & 6 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.