From e8bbdd67715a0ba3248ce751c3c934fafcd53de4 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 13 May 2021 19:09:43 +0200 Subject: [PATCH] (fix) more robust preprocess source maps In recent Svelte versions, preprocessing also returns a source map. If the user's version is high enough, use that instead of our own (now deprecated) logic. This provides more robust source mapping. #934 --- packages/language-server/src/importPackage.ts | 6 +- .../src/plugins/svelte/SvelteDocument.ts | 126 +++++++++++--- .../plugins/svelte/SvelteDocument.test.ts | 13 +- .../svelte/features/getCodeAction.test.ts | 156 ++++++++++++++++++ .../svelte/features/getDiagnostics.test.ts | 20 +-- .../svelte-ignore-code-action-ts.svelte | 16 ++ 6 files changed, 296 insertions(+), 41 deletions(-) create mode 100644 packages/language-server/test/plugins/svelte/testfiles/svelte-ignore-code-action-ts.svelte diff --git a/packages/language-server/src/importPackage.ts b/packages/language-server/src/importPackage.ts index 91e462229..6d18e952b 100644 --- a/packages/language-server/src/importPackage.ts +++ b/packages/language-server/src/importPackage.ts @@ -26,9 +26,9 @@ export function getPackageInfo(packageName: string, fromPath: string) { path: dirname(packageJSONPath), version: { full: version, - major, - minor, - patch + major: Number(major), + minor: Number(minor), + patch: Number(patch) } }; } diff --git a/packages/language-server/src/plugins/svelte/SvelteDocument.ts b/packages/language-server/src/plugins/svelte/SvelteDocument.ts index 2d489bf6e..76cf40c41 100644 --- a/packages/language-server/src/plugins/svelte/SvelteDocument.ts +++ b/packages/language-server/src/plugins/svelte/SvelteDocument.ts @@ -1,9 +1,9 @@ import { SourceMapConsumer } from 'source-map'; -import { PreprocessorGroup, Processed } from 'svelte/types/compiler/preprocess/types'; import type { compile } from 'svelte/compiler'; import { CompileOptions } from 'svelte/types/compiler/interfaces'; - +import { PreprocessorGroup, Processed } from 'svelte/types/compiler/preprocess/types'; import { Position } from 'vscode-languageserver'; +import { getPackageInfo, importSvelte } from '../../importPackage'; import { Document, DocumentMapper, @@ -17,7 +17,7 @@ import { SourceMapDocumentMapper, TagInformation } from '../../lib/documents'; -import { importSvelte } from '../../importPackage'; +import { SvelteConfig } from '../../lib/documents/configLoader'; import { isNotNullOrUndefined } from '../../utils'; export type SvelteCompileResult = ReturnType; @@ -33,7 +33,7 @@ type PositionMapper = Pick { + async getTranspiled(): Promise { if (!this.transpiledDoc) { - this.transpiledDoc = await TranspiledSvelteDocument.create( - this.parent, - ( + const { + version: { major, minor } + } = getPackageInfo('svelte', this.getFilePath()); + + if (major > 3 || (major === 3 && minor >= 32)) { + this.transpiledDoc = await TranspiledSvelteDocument.create( + this.parent, await this.config - )?.preprocess - ); + ); + } else { + this.transpiledDoc = await FallbackTranspiledSvelteDocument.create( + this.parent, + ( + await this.config + )?.preprocess + ); + } } return this.transpiledDoc; } @@ -101,7 +112,67 @@ export class SvelteDocument { } } -export class TranspiledSvelteDocument implements PositionMapper { +export interface ITranspiledSvelteDocument extends PositionMapper { + getText(): string; + destroy(): void; +} + +export class TranspiledSvelteDocument implements ITranspiledSvelteDocument { + static async create(document: Document, config: SvelteConfig | undefined) { + if (!config?.preprocess) { + return new TranspiledSvelteDocument(document.getText()); + } + + const filename = document.getFilePath() || ''; + const svelte = importSvelte(filename); + const preprocessed = await svelte.preprocess(document.getText(), config?.preprocess || [], { + filename + }); + + if (preprocessed.code === document.getText()) { + return new TranspiledSvelteDocument(document.getText()); + } + + return new TranspiledSvelteDocument( + preprocessed.code, + preprocessed.map + ? new SourceMapDocumentMapper( + await createSourceMapConsumer(preprocessed.map), + // The "sources" array only contains the Svelte filename, not its path. + // For getting generated positions, the sourcemap consumer wants an exact match + // of the source filepath. Therefore only pass in the filename here. + filename.replace(/\\/g, '/').split('/').pop() || '' + ) + : undefined + ); + } + + constructor(private code: string, private mapper?: SourceMapDocumentMapper) {} + + getOriginalPosition(generatedPosition: Position): Position { + return this.mapper?.getOriginalPosition(generatedPosition) || generatedPosition; + } + + getText() { + return this.code; + } + + getGeneratedPosition(originalPosition: Position): Position { + return this.mapper?.getGeneratedPosition(originalPosition) || originalPosition; + } + + destroy() { + this.mapper?.destroy(); + } +} + +/** + * Only used when the user has an old Svelte version installed where source map support + * for preprocessors is not built in yet. + * This fallback version does not map correctly when there's both a module and instance script. + * It isn't worth fixing these cases though now that Svelte ships a preprocessor with source maps. + */ +export class FallbackTranspiledSvelteDocument implements ITranspiledSvelteDocument { static async create( document: Document, preprocessors: PreprocessorGroup | PreprocessorGroup[] = [] @@ -121,7 +192,12 @@ export class TranspiledSvelteDocument implements PositionMapper { processedStyles ); - return new TranspiledSvelteDocument(document, transpiled, scriptMapper, styleMapper); + return new FallbackTranspiledSvelteDocument( + document, + transpiled, + scriptMapper, + styleMapper + ); } private fragmentInfos = [this.scriptMapper?.fragmentInfo, this.styleMapper?.fragmentInfo] @@ -252,23 +328,13 @@ export class SvelteFragmentMapper implements PositionMapper { async (parent, processedSingle) => processedSingle?.map ? new SourceMapDocumentMapper( - await new SourceMapConsumer(normalizeMap(processedSingle.map)), + await createSourceMapConsumer(processedSingle.map), originalDoc.uri, await parent ) : new IdentityMapper(originalDoc.uri, await parent), Promise.resolve(undefined) ); - - function normalizeMap(map: any) { - // We don't know what we get, could be a stringified sourcemap, - // or a class which has the required properties on it, or a class - // which we need to call toString() on to get the correct format. - if (typeof map === 'string' || map.version) { - return map; - } - return map.toString(); - } } private constructor( @@ -380,3 +446,17 @@ async function transpile( return { transpiled, processedScripts, processedStyles }; } + +async function createSourceMapConsumer(map: any): Promise { + return new SourceMapConsumer(normalizeMap(map)); + + function normalizeMap(map: any) { + // We don't know what we get, could be a stringified sourcemap, + // or a class which has the required properties on it, or a class + // which we need to call toString() on to get the correct format. + if (typeof map === 'string' || map.version) { + return map; + } + return map.toString(); + } +} diff --git a/packages/language-server/test/plugins/svelte/SvelteDocument.test.ts b/packages/language-server/test/plugins/svelte/SvelteDocument.test.ts index be020d53f..a4ff8456e 100644 --- a/packages/language-server/test/plugins/svelte/SvelteDocument.test.ts +++ b/packages/language-server/test/plugins/svelte/SvelteDocument.test.ts @@ -5,7 +5,7 @@ import { Document } from '../../../src/lib/documents'; import * as importPackage from '../../../src/importPackage'; import { SvelteDocument, - TranspiledSvelteDocument + ITranspiledSvelteDocument } from '../../../src/plugins/svelte/SvelteDocument'; import { configLoader, SvelteConfig } from '../../../src/lib/documents/configLoader'; import { Preprocessor } from 'svelte/types/compiler/preprocess/types'; @@ -33,7 +33,7 @@ describe('Svelte Document', () => { assert.strictEqual(svelteDoc.getText(), parent.getText()); }); - describe('#transpiled', () => { + describe('#transpiled (fallback)', () => { async function setupTranspiledWithStringSourceMap() { const stringSourceMapScript = () => ({ code: '', @@ -93,7 +93,10 @@ describe('Svelte Document', () => { }); // stub svelte preprocess and getOriginalPosition - // to fake a source mapping process + // to fake a source mapping process with the fallback version + sinon + .stub(importPackage, 'getPackageInfo') + .returns({ path: '', version: { full: '', major: 3, minor: 31, patch: 0 } }); sinon.stub(importPackage, 'importSvelte').returns({ preprocess: (text, preprocessor) => { preprocessor = Array.isArray(preprocessor) ? preprocessor : [preprocessor]; @@ -111,7 +114,7 @@ describe('Svelte Document', () => { parse: null }); const transpiled = await svelteDoc.getTranspiled(); - const scriptSourceMapper = (transpiled.scriptMapper).sourceMapper; + const scriptSourceMapper = (transpiled).scriptMapper.sourceMapper; // hacky reset of method because mocking the SourceMap constructor is an impossible task scriptSourceMapper.getOriginalPosition = ({ line, character }: Position) => ({ line: line - 1, @@ -127,7 +130,7 @@ describe('Svelte Document', () => { } function assertCanMapBackAndForth( - transpiled: TranspiledSvelteDocument, + transpiled: ITranspiledSvelteDocument, generatedPosition: Position, originalPosition: Position ) { diff --git a/packages/language-server/test/plugins/svelte/features/getCodeAction.test.ts b/packages/language-server/test/plugins/svelte/features/getCodeAction.test.ts index 6f014a87d..8b0b35996 100644 --- a/packages/language-server/test/plugins/svelte/features/getCodeAction.test.ts +++ b/packages/language-server/test/plugins/svelte/features/getCodeAction.test.ts @@ -258,6 +258,162 @@ describe('SveltePlugin#getCodeAction', () => { }); }); + describe('It should provide svelte ignore code actions (TypeScript)', () => { + const svelteIgnoreCodeAction = 'svelte-ignore-code-action-ts.svelte'; + + it('should provide ignore comment', async () => { + ( + await expectCodeActionFor(svelteIgnoreCodeAction, { + diagnostics: [ + { + severity: DiagnosticSeverity.Warning, + code: 'a11y-missing-attribute', + range: Range.create( + { line: 7, character: 0 }, + { line: 7, character: 6 } + ), + message: '', + source: 'svelte' + } + ] + }) + ).toEqual([ + { + edit: { + documentChanges: [ + { + edits: [ + { + // eslint-disable-next-line max-len + newText: `${EOL}`, + range: { + end: { + character: 0, + line: 7 + }, + start: { + character: 0, + line: 7 + } + } + } + ], + textDocument: { + uri: getUri(svelteIgnoreCodeAction), + version: null + } + } + ] + }, + title: '(svelte) Disable a11y-missing-attribute for this line', + kind: 'quickfix' + } + ]); + }); + + it('should provide ignore comment with indent', async () => { + ( + await expectCodeActionFor(svelteIgnoreCodeAction, { + diagnostics: [ + { + severity: DiagnosticSeverity.Warning, + code: 'a11y-missing-attribute', + range: Range.create( + { line: 10, character: 4 }, + { line: 10, character: 11 } + ), + message: '', + source: 'svelte' + } + ] + }) + ).toEqual([ + { + edit: { + documentChanges: [ + { + edits: [ + { + newText: `${' '.repeat( + 4 + )}${EOL}`, + range: { + end: { + character: 0, + line: 10 + }, + start: { + character: 0, + line: 10 + } + } + } + ], + textDocument: { + uri: getUri(svelteIgnoreCodeAction), + version: null + } + } + ] + }, + title: '(svelte) Disable a11y-missing-attribute for this line', + kind: 'quickfix' + } + ]); + }); + + it('should provide ignore comment with indent of parent tag', async () => { + ( + await expectCodeActionFor(svelteIgnoreCodeAction, { + diagnostics: [ + { + severity: DiagnosticSeverity.Warning, + code: 'a11y-invalid-attribute', + range: Range.create( + { line: 13, character: 8 }, + { line: 13, character: 15 } + ), + message: '', + source: 'svelte' + } + ] + }) + ).toEqual([ + { + edit: { + documentChanges: [ + { + edits: [ + { + newText: `${' '.repeat( + 4 + )}${EOL}`, + range: { + end: { + character: 0, + line: 12 + }, + start: { + character: 0, + line: 12 + } + } + } + ], + textDocument: { + uri: getUri(svelteIgnoreCodeAction), + version: null + } + } + ] + }, + title: '(svelte) Disable a11y-invalid-attribute for this line', + kind: 'quickfix' + } + ]); + }); + }); + describe('#extractComponent', async () => { const scriptContent = ` + + + +{#if true} + + + about +{/if}