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

breaking: svelte-check v4 #2453

Merged
merged 12 commits into from
Aug 27, 2024
Merged
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 @@ -47,6 +47,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 @@ -61,7 +63,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 @@ -84,12 +86,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('/.');
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
})
.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 @@ -296,4 +305,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 @@ -98,14 +121,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 @@ -139,11 +162,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 @@ -153,7 +173,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 @@ -167,7 +187,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
14 changes: 9 additions & 5 deletions packages/svelte-check/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,19 @@
"url": "https://github.com/sveltejs/language-tools/issues"
},
"homepage": "https://github.com/sveltejs/language-tools#readme",
"engines": {
"node": ">= 18.0.0"
},
"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",
"typescript": "^5.0.3"
"sade": "^1.7.4"
},
"peerDependencies": {
"svelte": "^3.55.0 || ^4.0.0-next.0 || ^4.0.0 || ^5.0.0-next.0"
"svelte": "^4.0.0 || ^5.0.0-next.0",
"typescript": ">=5.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Should we support some versions of Typescript 4? It might be nice for users who haven't been able to upgrade yet if it's not too hard for us

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reminding me to bump the Svelte peer dependency to v4 😄 And since v4 has an implicit dependency requirement on TS >=5 we should, too. Better to nudge users towards upgrading

Copy link
Member

Choose a reason for hiding this comment

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

I think users will have plenty of reason to upgrade on their own since Svelte 5 is so much better. But it'd probably be appreciated if we let them do it in their own time instead of forcing them. It can make things easier to have wider support as you have more flexibility around what order to upgrade various libraries

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't forcing people to jump to Svelte 5, only Svelte 4 - which you need to do before upgrading to 5 anyway.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I know. I was just saying people would want to upgrade to Svelte 4 in order to get to 5 so we don't need to push them too hard

},
"scripts": {
"build": "rollup -c && node ./dist/src/index.js --workspace ./test --tsconfig ./tsconfig.json",
Expand All @@ -46,11 +49,12 @@
"@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",
"svelte": "^3.57.0",
"svelte-language-server": "workspace:*",
"typescript": "^5.5.2",
"vscode-languageserver": "8.0.2",
"vscode-languageserver-protocol": "3.17.2",
"vscode-languageserver-types": "3.17.2",
Expand Down
1 change: 0 additions & 1 deletion packages/svelte-check/rollup.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ export default [
'sade',
'svelte',
'svelte/compiler',
'svelte-preprocess',
'@jridgewell/trace-mapping'
// import-fresh removed some time ago, no dependency uses it anymore.
// if it creeps back in check if the dependency uses a version that
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);
}

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');
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
})
.withPathSeparator('/')
.withFullPaths()
.crawl(workspaceUri.fsPath)
.withPromise();

for (const absFilePath of absFilePaths) {
const text = fs.readFileSync(absFilePath, 'utf-8');
Expand Down
20 changes: 13 additions & 7 deletions packages/svelte-check/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,19 @@ export function parseOptions(cb: (opts: SvelteCheckCliOptions) => any) {
)
.action((opts) => {
const workspaceUri = getWorkspaceUri(opts);
const tsconfig = getTsconfig(opts, workspaceUri.fsPath);

if (opts.ignore && tsconfig) {
throwError('`--ignore` only has an effect when using `--no-tsconfig`');
}

cb({
workspaceUri,
outputFormat: getOutputFormat(opts),
watch: !!opts.watch,
preserveWatchOutput: !!opts.preserveWatchOutput,
tsconfig: getTsconfig(opts, workspaceUri.fsPath),
filePathsToIgnore: getFilepathsToIgnore(opts),
tsconfig,
filePathsToIgnore: opts.ignore?.split(',') || [],
failOnWarnings: !!opts['fail-on-warnings'],
compilerWarnings: getCompilerWarnings(opts),
diagnosticSources: getDiagnosticSources(opts),
Expand Down Expand Up @@ -141,11 +147,15 @@ function getTsconfig(myArgs: Record<string, any>, workspacePath: string) {
tsconfig = path.join(workspacePath, tsconfig);
}
if (tsconfig && !fs.existsSync(tsconfig)) {
throw new Error('Could not find tsconfig/jsconfig file at ' + myArgs.tsconfig);
throwError('Could not find tsconfig/jsconfig file at ' + myArgs.tsconfig);
}
return tsconfig;
}

function throwError(msg: string) {
throw new Error('Invalid svelte-check CLI args: ' + msg);
}

function getCompilerWarnings(opts: Record<string, any>) {
return stringToObj(opts['compiler-warnings']);

Expand Down Expand Up @@ -180,10 +190,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
Loading