From a9a0ad313e8b397d1dc42100911ae94e77df5e84 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Mon, 29 Jun 2020 10:18:06 +0200 Subject: [PATCH] (feat) better error diagnostics (#240) * (feat) better error diagnostics Narrow down in which step the error occurs and try to extract better error messages. #86, #232, #129 * (feat) show svelte.config.js errors * cleanup * tests * fix error msg --- .../src/plugins/svelte/SvelteDocument.ts | 37 ++- .../src/plugins/svelte/SveltePlugin.ts | 165 +++-------- .../plugins/svelte/features/getDiagnostics.ts | 242 ++++++++++++++++ .../svelte/features/getDiagnostics.test.ts | 262 ++++++++++++++++++ 4 files changed, 569 insertions(+), 137 deletions(-) create mode 100644 packages/language-server/src/plugins/svelte/features/getDiagnostics.ts create mode 100644 packages/language-server/test/plugins/svelte/features/getDiagnostics.test.ts diff --git a/packages/language-server/src/plugins/svelte/SvelteDocument.ts b/packages/language-server/src/plugins/svelte/SvelteDocument.ts index 5715e9b08..4c5a96db6 100644 --- a/packages/language-server/src/plugins/svelte/SvelteDocument.ts +++ b/packages/language-server/src/plugins/svelte/SvelteDocument.ts @@ -21,6 +21,12 @@ export type SvelteCompileResult = ReturnType; export interface SvelteConfig extends CompileOptions { preprocess?: PreprocessorGroup; + loadConfigError?: any; +} + +export enum TranspileErrorSource { + Script = 'Script', + Style = 'Style', } /** @@ -78,7 +84,10 @@ export class SvelteDocument { private getCompileOptions() { const config = { ...this.config }; - delete config.preprocess; // svelte compiler throws an error if we don't do this + // svelte compiler throws an error if we don't do this + delete config.preprocess; + delete config.loadConfigError; + return config; } @@ -272,21 +281,31 @@ async function transpile(document: Document, preprocessors: PreprocessorGroup = if (preprocessors.script) { preprocessor.script = async (args: any) => { - const res = await preprocessors.script!(args); - if (res && res.map) { - processedScript = res; + try { + const res = await preprocessors.script!(args); + if (res && res.map) { + processedScript = res; + } + return res; + } catch (e) { + e.__source = TranspileErrorSource.Script; + throw e; } - return res; }; } if (preprocessors.style) { preprocessor.style = async (args: any) => { - const res = await preprocessors.style!(args); - if (res && res.map) { - processedStyle = res; + try { + const res = await preprocessors.style!(args); + if (res && res.map) { + processedStyle = res; + } + return res; + } catch (e) { + e.__source = TranspileErrorSource.Style; + throw e; } - return res; }; } diff --git a/packages/language-server/src/plugins/svelte/SveltePlugin.ts b/packages/language-server/src/plugins/svelte/SveltePlugin.ts index a14da83e9..b7fc289b7 100644 --- a/packages/language-server/src/plugins/svelte/SveltePlugin.ts +++ b/packages/language-server/src/plugins/svelte/SveltePlugin.ts @@ -1,31 +1,31 @@ import { cosmiconfig } from 'cosmiconfig'; -import { CompileOptions, Warning } from 'svelte/types/compiler/interfaces'; +import { CompileOptions } from 'svelte/types/compiler/interfaces'; import { + CodeAction, + CodeActionContext, CompletionList, Diagnostic, - DiagnosticSeverity, Hover, Position, Range, TextEdit, - CodeActionContext, - CodeAction, } from 'vscode-languageserver'; -import { Document, isInTag, mapDiagnosticToOriginal } from '../../lib/documents'; +import { Document } from '../../lib/documents'; +import { Logger } from '../../logger'; import { LSConfigManager, LSSvelteConfig } from '../../ls-config'; import { importPrettier, importSveltePreprocess } from '../importPackage'; import { + CodeActionsProvider, CompletionsProvider, DiagnosticsProvider, FormattingProvider, HoverProvider, - CodeActionsProvider, } from '../interfaces'; +import { getCodeActions } from './features/getCodeActions'; import { getCompletions } from './features/getCompletions'; +import { getDiagnostics } from './features/getDiagnostics'; import { getHoverInfo } from './features/getHoverInfo'; -import { SvelteDocument, SvelteConfig, SvelteCompileResult } from './SvelteDocument'; -import { Logger } from '../../logger'; -import { getCodeActions } from './features/getCodeActions'; +import { SvelteCompileResult, SvelteConfig, SvelteDocument } from './SvelteDocument'; const DEFAULT_OPTIONS: CompileOptions = { dev: true, @@ -34,10 +34,6 @@ const DEFAULT_OPTIONS: CompileOptions = { const NO_GENERATE: CompileOptions = { generate: false, }; - -const scssNodeRuntimeHint = - 'If you use SCSS, it may be necessary to add the path to your NODE runtime to the setting `svelte.language-server.runtime`, or use `sass` instead of `node-sass`. '; - export class SveltePlugin implements DiagnosticsProvider, @@ -58,63 +54,7 @@ export class SveltePlugin return []; } - try { - return await this.tryGetDiagnostics(document); - } catch (error) { - Logger.error('Preprocessing failed'); - Logger.error(error); - // Preprocessing could fail if packages like less/sass/babel cannot be resolved - // when our fallback-version of svelte-preprocess is used. - // Add a warning about a broken svelte.configs.js/preprocessor setup - // Also add svelte-preprocess error message. - const errorMsg = - error instanceof Error && error.message.startsWith('Cannot find any of modules') - ? error.message + '. ' - : ''; - const hint = - error instanceof Error && error.message.includes('node-sass') - ? scssNodeRuntimeHint - : ''; - return [ - { - message: - errorMsg + - "The file cannot be parsed because script or style require a preprocessor that doesn't seem to be setup. " + - 'Did you setup a `svelte.config.js`? ' + - hint + - 'See https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#using-with-preprocessors for more info.', - range: Range.create(Position.create(0, 0), Position.create(0, 5)), - severity: DiagnosticSeverity.Warning, - source: 'svelte', - }, - ]; - } - } - - private async tryGetDiagnostics(document: Document): Promise { - const svelteDoc = await this.getSvelteDoc(document); - const transpiled = await svelteDoc.getTranspiled(); - - try { - const res = await svelteDoc.getCompiled(); - return (((res.stats as any).warnings || res.warnings || []) as Warning[]) - .map((warning) => { - const start = warning.start || { line: 1, column: 0 }; - const end = warning.end || start; - return { - range: Range.create(start.line - 1, start.column, end.line - 1, end.column), - message: warning.message, - severity: DiagnosticSeverity.Warning, - source: 'svelte', - code: warning.code, - }; - }) - .map((diag) => mapDiagnosticToOriginal(transpiled, diag)); - } catch (err) { - return (await this.createParserErrorDiagnostic(err, document)).map((diag) => - mapDiagnosticToOriginal(transpiled, diag), - ); - } + return getDiagnostics(document, await this.getSvelteDoc(document)); } async getCompiledResult(document: Document): Promise { @@ -126,63 +66,6 @@ export class SveltePlugin } } - private async createParserErrorDiagnostic(error: any, document: Document) { - const start = error.start || { line: 1, column: 0 }; - const end = error.end || start; - const diagnostic: Diagnostic = { - range: Range.create(start.line - 1, start.column, end.line - 1, end.column), - message: error.message, - severity: DiagnosticSeverity.Error, - source: 'svelte', - code: error.code, - }; - - if (diagnostic.message.includes('expected')) { - const isInStyle = isInTag(diagnostic.range.start, document.styleInfo); - const isInScript = isInTag(diagnostic.range.start, document.scriptInfo); - - if (isInStyle || isInScript) { - diagnostic.message += - '. If you expect this syntax to work, here are some suggestions: '; - if (isInScript) { - diagnostic.message += - 'If you use typescript with `svelte-preprocessor`, did you add `lang="typescript"` to your `script` tag? '; - } else { - diagnostic.message += - 'If you use less/SCSS with `svelte-preprocessor`, did you add `lang="scss"`/`lang="less"` to you `style` tag? ' + - scssNodeRuntimeHint; - } - diagnostic.message += - 'Did you setup a `svelte.config.js`? ' + - 'See https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#using-with-preprocessors for more info.'; - } - } - - return [diagnostic]; - } - - private useFallbackPreprocessor(document: Document, foundConfig: boolean) { - if ( - document.styleInfo?.attributes.lang || - document.styleInfo?.attributes.type || - document.scriptInfo?.attributes.lang || - document.scriptInfo?.attributes.type - ) { - Logger.log( - (foundConfig - ? 'Found svelte.config.js but there was an error loading it. ' - : 'No svelte.config.js found but one is needed. ') + - 'Using https://github.com/sveltejs/svelte-preprocess as fallback', - ); - return { - preprocess: importSveltePreprocess(document.getFilePath() || '')({ - typescript: { transpileOnly: true }, - }), - }; - } - return {}; - } - async formatDocument(document: Document): Promise { if (!this.featureEnabled('format')) { return []; @@ -251,7 +134,10 @@ export class SveltePlugin if (!svelteDoc || svelteDoc.version !== document.version) { svelteDoc?.destroyTranspiled(); // Reuse previous config. Assumption: Config does not change often (if at all). - const config = svelteDoc?.config || (await this.loadConfig(document)); + const config = + svelteDoc?.config && !svelteDoc.config.loadConfigError + ? svelteDoc.config + : await this.loadConfig(document); svelteDoc = new SvelteDocument(document, config); this.docManager.set(document, svelteDoc); } @@ -274,7 +160,30 @@ export class SveltePlugin ...DEFAULT_OPTIONS, ...this.useFallbackPreprocessor(document, true), ...NO_GENERATE, + loadConfigError: err, }; } } + + private useFallbackPreprocessor(document: Document, foundConfig: boolean) { + if ( + document.styleInfo?.attributes.lang || + document.styleInfo?.attributes.type || + document.scriptInfo?.attributes.lang || + document.scriptInfo?.attributes.type + ) { + Logger.log( + (foundConfig + ? 'Found svelte.config.js but there was an error loading it. ' + : 'No svelte.config.js found but one is needed. ') + + 'Using https://github.com/sveltejs/svelte-preprocess as fallback', + ); + return { + preprocess: importSveltePreprocess(document.getFilePath() || '')({ + typescript: { transpileOnly: true }, + }), + }; + } + return {}; + } } diff --git a/packages/language-server/src/plugins/svelte/features/getDiagnostics.ts b/packages/language-server/src/plugins/svelte/features/getDiagnostics.ts new file mode 100644 index 000000000..08e9af5e1 --- /dev/null +++ b/packages/language-server/src/plugins/svelte/features/getDiagnostics.ts @@ -0,0 +1,242 @@ +import { Warning } from 'svelte/types/compiler/interfaces'; +import { Diagnostic, DiagnosticSeverity, Position, Range } from 'vscode-languageserver'; +import { Document, isInTag, mapDiagnosticToOriginal } from '../../../lib/documents'; +import { Logger } from '../../../logger'; +import { SvelteDocument, TranspileErrorSource } from '../SvelteDocument'; + +/** + * Returns diagnostics from the svelte compiler. + * Also tries to return errors at correct position if transpilation/preprocessing fails. + */ +export async function getDiagnostics( + document: Document, + svelteDoc: SvelteDocument, +): Promise { + try { + return await tryGetDiagnostics(document, svelteDoc); + } catch (error) { + return getPreprocessErrorDiagnostics(document, svelteDoc, error); + } +} + +/** + * Try to transpile and compile the svelte file and return diagnostics. + */ +async function tryGetDiagnostics( + document: Document, + svelteDoc: SvelteDocument, +): Promise { + const transpiled = await svelteDoc.getTranspiled(); + + try { + const res = await svelteDoc.getCompiled(); + return (((res.stats as any).warnings || res.warnings || []) as Warning[]) + .map((warning) => { + const start = warning.start || { line: 1, column: 0 }; + const end = warning.end || start; + return { + range: Range.create(start.line - 1, start.column, end.line - 1, end.column), + message: warning.message, + severity: DiagnosticSeverity.Warning, + source: 'svelte', + code: warning.code, + }; + }) + .map((diag) => mapDiagnosticToOriginal(transpiled, diag)); + } catch (err) { + return (await createParserErrorDiagnostic(err, document)).map((diag) => + mapDiagnosticToOriginal(transpiled, diag), + ); + } +} + +/** + * Try to infer a nice diagnostic error message from the compilation error. + */ +async function createParserErrorDiagnostic(error: any, document: Document) { + const start = error.start || { line: 1, column: 0 }; + const end = error.end || start; + const diagnostic: Diagnostic = { + range: Range.create(start.line - 1, start.column, end.line - 1, end.column), + message: error.message, + severity: DiagnosticSeverity.Error, + source: 'svelte', + code: error.code, + }; + + if (diagnostic.message.includes('expected')) { + const isInStyle = isInTag(diagnostic.range.start, document.styleInfo); + const isInScript = isInTag(diagnostic.range.start, document.scriptInfo); + + if (isInStyle || isInScript) { + diagnostic.message += + '\n\nIf you expect this syntax to work, here are some suggestions: '; + if (isInScript) { + diagnostic.message += + '\nIf you use typescript with `svelte-preprocessor`, did you add `lang="typescript"` to your `script` tag? '; + } else { + diagnostic.message += + '\nIf you use less/SCSS with `svelte-preprocessor`, did you add `lang="scss"`/`lang="less"` to you `style` tag? ' + + scssNodeRuntimeHint; + } + diagnostic.message += + '\nDid you setup a `svelte.config.js`? ' + + '\nSee https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#using-with-preprocessors for more info.'; + } + } + + return [diagnostic]; +} + +/** + * Try to infer a nice diagnostic error message from the transpilation error. + */ +function getPreprocessErrorDiagnostics( + document: Document, + svelteDoc: SvelteDocument, + error: any, +): Diagnostic[] { + Logger.error('Preprocessing failed'); + Logger.error(error); + + if (svelteDoc.config.loadConfigError) { + return getConfigLoadErrorDiagnostics(svelteDoc.config.loadConfigError); + } + + if (document.styleInfo && error.__source === TranspileErrorSource.Style) { + return getStyleErrorDiagnostics(error, document); + } + + if ( + (document.scriptInfo || document.moduleScriptInfo) && + error.__source === TranspileErrorSource.Script + ) { + return getScriptErrorDiagnostics(error, document); + } + + return getOtherErrorDiagnostics(error); +} + +function getConfigLoadErrorDiagnostics(error: any): Diagnostic[] { + return [ + { + message: 'Error in svelte.config.js\n\n' + error, + range: Range.create(Position.create(0, 0), Position.create(0, 5)), + severity: DiagnosticSeverity.Error, + source: 'svelte', + }, + ]; +} + +/** + * Try to infer a nice diagnostic error message from the transpilation error. + */ +function getStyleErrorDiagnostics(error: any, document: Document): Diagnostic[] { + return [ + { + message: getStyleErrorMessage(), + range: getStyleErrorRange(), + severity: DiagnosticSeverity.Error, + source: 'svelte(style)', + }, + ]; + + function getStyleErrorMessage() { + if (isSveltePreprocessCannotFindModulesError(error)) { + const hint = error.message.includes('node-sass') ? scssNodeRuntimeHint : ''; + return getErrorMessage(error.message, 'style', hint); + } + + return ( + error.formatted /* sass error messages have this */ || + error.message || + 'Style error. Transpilation failed.' + ); + } + + function getStyleErrorRange() { + const lineOffset = document.styleInfo?.startPos.line || 0; + const position = + typeof error?.column === 'number' && typeof error?.line === 'number' + ? // Some preprocessors like sass or less return error objects with these attributes. + // Use it to display a nice error message. + Position.create(lineOffset + error.line - 1, error.column) + : document.styleInfo?.startPos || Position.create(0, 0); + return Range.create(position, position); + } +} + +/** + * Try to infer a nice diagnostic error message from the transpilation error. + */ +function getScriptErrorDiagnostics(error: any, document: Document): Diagnostic[] { + return [ + { + message: getScriptErrorMessage(), + range: getScriptErrorRange(), + severity: DiagnosticSeverity.Error, + source: 'svelte(script)', + }, + ]; + + function getScriptErrorMessage() { + if (isSveltePreprocessCannotFindModulesError(error)) { + return getErrorMessage(error.message, 'script'); + } + + return error.message || 'Script error. Transpilation failed.'; + } + + function getScriptErrorRange() { + const position = + document.scriptInfo?.startPos || + document.moduleScriptInfo?.startPos || + Position.create(0, 0); + return Range.create(position, position); + } +} + +/** + * Try to infer a nice diagnostic error message from the transpilation error. + */ +function getOtherErrorDiagnostics(error: any): Diagnostic[] { + return [ + { + message: getOtherErrorMessage(), + range: Range.create(Position.create(0, 0), Position.create(0, 5)), + severity: DiagnosticSeverity.Warning, + source: 'svelte', + }, + ]; + + function getOtherErrorMessage() { + if (isSveltePreprocessCannotFindModulesError(error)) { + return getErrorMessage(error.message, 'it'); + } + + return error.message || 'Error. Transpilation failed.'; + } +} + +/** + * Preprocessing could fail if packages cannot be resolved. + * A warning about a broken svelte.configs.js/preprocessor setup should be added then. + */ +function isSveltePreprocessCannotFindModulesError(error: any): error is Error { + return error instanceof Error && error.message.startsWith('Cannot find any of modules'); +} + +function getErrorMessage(error: any, source: string, hint = '') { + return ( + error + + '\n\nThe file cannot be parsed because ' + + source + + " requires a preprocessor that doesn't seem to be setup or failed during setup. " + + 'Did you setup a `svelte.config.js`? ' + + hint + + '\n\nSee https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#using-with-preprocessors for more info.' + ); +} + +const scssNodeRuntimeHint = + 'If you use SCSS, it may be necessary to add the path to your NODE runtime to the setting `svelte.language-server.runtime`, or use `sass` instead of `node-sass`. '; diff --git a/packages/language-server/test/plugins/svelte/features/getDiagnostics.test.ts b/packages/language-server/test/plugins/svelte/features/getDiagnostics.test.ts new file mode 100644 index 000000000..acfadc429 --- /dev/null +++ b/packages/language-server/test/plugins/svelte/features/getDiagnostics.test.ts @@ -0,0 +1,262 @@ +import * as assert from 'assert'; +import { Diagnostic, DiagnosticSeverity, Position } from 'vscode-languageserver'; +import { Document } from '../../../../src/lib/documents'; +import { getDiagnostics } from '../../../../src/plugins/svelte/features/getDiagnostics'; +import { + SvelteConfig, + SvelteDocument, + TranspileErrorSource, +} from '../../../../src/plugins/svelte/SvelteDocument'; + +describe('SveltePlugin#getDiagnostics', () => { + async function expectDiagnosticsFor( + getTranspiled: any, + getCompiled: any, + config: Partial, + ) { + const document = new Document('', '\n'); + const svelteDoc: SvelteDocument = { getTranspiled, getCompiled, config }; + const result = await getDiagnostics(document, svelteDoc); + return { + toEqual: (expected: Diagnostic[]) => assert.deepStrictEqual(result, expected), + }; + } + + it('expect svelte.config.js error', async () => { + ( + await expectDiagnosticsFor( + () => { + throw new Error(); + }, + () => '', + { loadConfigError: new Error('svelte.config.js') }, + ) + ).toEqual([ + { + message: 'Error in svelte.config.js\n\nError: svelte.config.js', + range: { + start: { + character: 0, + line: 0, + }, + end: { + character: 5, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte', + }, + ]); + }); + + it('expect script transpilation error', async () => { + ( + await expectDiagnosticsFor( + () => { + const e: any = new Error('Script'); + e.__source = TranspileErrorSource.Script; + throw e; + }, + () => '', + {}, + ) + ).toEqual([ + { + message: 'Script', + range: { + start: { + character: 8, + line: 0, + }, + end: { + character: 8, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte(script)', + }, + ]); + }); + + it('expect style transpilation error', async () => { + ( + await expectDiagnosticsFor( + () => { + const e: any = new Error('Style'); + e.__source = TranspileErrorSource.Style; + throw e; + }, + () => '', + {}, + ) + ).toEqual([ + { + message: 'Style', + range: { + start: { + character: 7, + line: 1, + }, + end: { + character: 7, + line: 1, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte(style)', + }, + ]); + }); + + it('expect style transpilation error with line/columns', async () => { + ( + await expectDiagnosticsFor( + () => { + const e: any = new Error('Style'); + e.line = 1; + e.column = 0; + e.__source = TranspileErrorSource.Style; + throw e; + }, + () => '', + {}, + ) + ).toEqual([ + { + message: 'Style', + range: { + start: { + character: 0, + line: 1, + }, + end: { + character: 0, + line: 1, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte(style)', + }, + ]); + }); + + it('expect compilation error', async () => { + ( + await expectDiagnosticsFor( + () => ({ + getOriginalPosition: () => Position.create(0, 0), + }), + () => { + const e: any = new Error('Compilation'); + e.message = 'ERROR'; + e.code = 123; + throw e; + }, + {}, + ) + ).toEqual([ + { + code: 123, + message: 'ERROR', + range: { + start: { + character: 0, + line: 0, + }, + end: { + character: 0, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte', + }, + ]); + }); + + it('expect compilation error with expected', async () => { + ( + await expectDiagnosticsFor( + () => ({ + getOriginalPosition: () => Position.create(0, 8), + }), + () => { + const e: any = new Error('Compilation'); + e.message = 'expected x to not be here'; + e.code = 123; + e.start = { line: 1, column: 8 }; + throw e; + }, + {}, + ) + ).toEqual([ + { + code: 123, + message: + 'expected x to not be here' + + '\n\nIf you expect this syntax to work, here are some suggestions: ' + + '\nIf you use typescript with `svelte-preprocessor`, did you add `lang="typescript"` to your `script` tag? ' + + '\nDid you setup a `svelte.config.js`? ' + + '\nSee https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#using-with-preprocessors for more info.', + range: { + start: { + character: 8, + line: 0, + }, + end: { + character: 8, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + source: 'svelte', + }, + ]); + }); + + it('expect warnings', async () => { + ( + await expectDiagnosticsFor( + () => ({ + getOriginalPosition: (pos: Position) => { + pos.line - 1; + return pos; + }, + }), + () => + Promise.resolve({ + stats: { + warnings: [ + { + start: { line: 1, column: 0 }, + end: { line: 1, column: 0 }, + message: 'warning', + code: 123, + }, + ], + }, + }), + {}, + ) + ).toEqual([ + { + code: 123, + message: 'warning', + range: { + start: { + character: 0, + line: 0, + }, + end: { + character: 0, + line: 0, + }, + }, + severity: DiagnosticSeverity.Warning, + source: 'svelte', + }, + ]); + }); +});