From b05b1f2c474967ddc59a19408fdbf506c1379026 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 30 Aug 2022 16:16:35 -0400 Subject: [PATCH 1/3] refactor: consolidate `diagnostics` funcs into single file - move `IDiagnostic` and `convertDiagnostic` from `tscache` to `print-diagnostics`, then rename it to `diagnostics.ts` - the diagnostic funcs being in `tscache` always felt like a strange place for them - especially when `parse-tsconfig` or `print-diagnostics` would import them from `tscache`, which sounded like an unrelated file - may want to move `diagnostics-format-host` into this file as well, as it is _only_ used in this file - leaving as is for now, limiting this change to a smaller one --- CONTRIBUTING.md | 2 +- ...iagnostics.spec.ts => diagnostics.spec.ts} | 6 ++-- src/{print-diagnostics.ts => diagnostics.ts} | 35 ++++++++++++++++++- src/index.ts | 4 +-- src/parse-tsconfig.ts | 3 +- src/tscache.ts | 35 +------------------ 6 files changed, 42 insertions(+), 43 deletions(-) rename __tests__/{print-diagnostics.spec.ts => diagnostics.spec.ts} (92%) rename src/{print-diagnostics.ts => diagnostics.ts} (55%) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdd4693a..90f3520d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -73,5 +73,5 @@ A useful resource as you dive deeper are the [unit tests](__tests__/). They're g 1. At this point, you may be ready to read the more complicated bits of [`index`](src/index.ts) in detail and see how it interacts with the other modules. - The [integration tests](__tests__/integration/) could be useful to review at this point as well. 1. Once you're pretty familiar with `index`, you can dive into some of the cache code in [`tscache`](src/tscache.ts) and [`rollingcache`](src/rollingcache.ts). -1. And finally, you can see some of the Rollup logging nuances in [`context`](src/context.ts) and then the TS logging nuances in [`print-diagnostics`](src/print-diagnostics.ts), and [`diagnostics-format-host`](src/diagnostics-format-host.ts) +1. And finally, you can see some of the Rollup logging nuances in [`context`](src/context.ts) and then the TS logging nuances in [`diagnostics`](src/diagnostics.ts), and [`diagnostics-format-host`](src/diagnostics-format-host.ts) - While these are necessary to the implementation, they are fairly ancillary to understanding and working with the codebase. diff --git a/__tests__/print-diagnostics.spec.ts b/__tests__/diagnostics.spec.ts similarity index 92% rename from __tests__/print-diagnostics.spec.ts rename to __tests__/diagnostics.spec.ts index 844a1f0d..141cc8bf 100644 --- a/__tests__/print-diagnostics.spec.ts +++ b/__tests__/diagnostics.spec.ts @@ -4,7 +4,7 @@ import { red } from "colors/safe"; import { makeContext } from "./fixtures/context"; import { setTypescriptModule } from "../src/tsproxy"; -import { printDiagnostics } from "../src/print-diagnostics"; +import { printDiagnostics } from "../src/diagnostics"; setTypescriptModule(ts); @@ -16,7 +16,7 @@ const diagnostic = { type: 'config' }; -test("print-diagnostics - categories", () => { +test("printDiagnostics - categories", () => { const context = makeContext(); printDiagnostics(context, [diagnostic]); @@ -38,7 +38,7 @@ test("print-diagnostics - categories", () => { expect(context.debug).toBeCalledTimes(0); }); -test("print-diagnostics - formatting / style", () => { +test("printDiagnostics - formatting / style", () => { const context = makeContext(); const category = "error"; // string version diff --git a/src/print-diagnostics.ts b/src/diagnostics.ts similarity index 55% rename from src/print-diagnostics.ts rename to src/diagnostics.ts index 868c09e0..d679f0b1 100644 --- a/src/print-diagnostics.ts +++ b/src/diagnostics.ts @@ -1,8 +1,41 @@ +import * as tsTypes from "typescript"; import { red, white, yellow } from "colors/safe"; import { tsModule } from "./tsproxy"; import { RollupContext } from "./context"; -import { IDiagnostics } from "./tscache"; +import { formatHost } from "./diagnostics-format-host"; + +export interface IDiagnostics +{ + flatMessage: string; + formatted: string; + fileLine?: string; + category: tsTypes.DiagnosticCategory; + code: number; + type: string; +} + +export function convertDiagnostic(type: string, data: tsTypes.Diagnostic[]): IDiagnostics[] +{ + return data.map((diagnostic) => + { + const entry: IDiagnostics = { + flatMessage: tsModule.flattenDiagnosticMessageText(diagnostic.messageText, "\n"), + formatted: tsModule.formatDiagnosticsWithColorAndContext(data, formatHost), + category: diagnostic.category, + code: diagnostic.code, + type, + }; + + if (diagnostic.file && diagnostic.start !== undefined) + { + const { line, character } = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start); + entry.fileLine = `${diagnostic.file.fileName}(${line + 1},${character + 1})`; + } + + return entry; + }); +} export function printDiagnostics(context: RollupContext, diagnostics: IDiagnostics[], pretty = true): void { diff --git a/src/index.ts b/src/index.ts index 8b8d552b..a4bcdb8c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -7,11 +7,11 @@ import findCacheDir from "find-cache-dir"; import { RollupContext, VerbosityLevel } from "./context"; import { LanguageServiceHost } from "./host"; -import { TsCache, convertDiagnostic, convertEmitOutput, getAllReferences, ICode } from "./tscache"; +import { TsCache, convertEmitOutput, getAllReferences, ICode } from "./tscache"; import { tsModule, setTypescriptModule } from "./tsproxy"; import { IOptions } from "./ioptions"; import { parseTsConfig } from "./parse-tsconfig"; -import { printDiagnostics } from "./print-diagnostics"; +import { convertDiagnostic, printDiagnostics } from "./diagnostics"; import { TSLIB, TSLIB_VIRTUAL, tslibSource, tslibVersion } from "./tslib"; import { createFilter } from "./get-options-overrides"; diff --git a/src/parse-tsconfig.ts b/src/parse-tsconfig.ts index 740938be..8b2d3aa3 100644 --- a/src/parse-tsconfig.ts +++ b/src/parse-tsconfig.ts @@ -3,8 +3,7 @@ import * as _ from "lodash"; import { tsModule } from "./tsproxy"; import { RollupContext } from "./context"; -import { printDiagnostics } from "./print-diagnostics"; -import { convertDiagnostic } from "./tscache"; +import { convertDiagnostic, printDiagnostics } from "./diagnostics"; import { getOptionsOverrides } from "./get-options-overrides"; import { IOptions } from "./ioptions"; diff --git a/src/tscache.ts b/src/tscache.ts index 9fabd21a..afa3e2de 100644 --- a/src/tscache.ts +++ b/src/tscache.ts @@ -9,7 +9,7 @@ import { RollupContext } from "./context"; import { RollingCache } from "./rollingcache"; import { ICache } from "./icache"; import { tsModule } from "./tsproxy"; -import { formatHost } from "./diagnostics-format-host"; +import { IDiagnostics, convertDiagnostic } from "./diagnostics"; export interface ICode { @@ -25,16 +25,6 @@ interface INodeLabel dirty: boolean; } -export interface IDiagnostics -{ - flatMessage: string; - formatted: string; - fileLine?: string; - category: tsTypes.DiagnosticCategory; - code: number; - type: string; -} - interface ITypeSnapshot { id: string; @@ -74,29 +64,6 @@ export function getAllReferences(importer: string, snapshot: tsTypes.IScriptSnap })); } -export function convertDiagnostic(type: string, data: tsTypes.Diagnostic[]): IDiagnostics[] -{ - return data.map((diagnostic) => - { - const entry: IDiagnostics = - { - flatMessage: tsModule.flattenDiagnosticMessageText(diagnostic.messageText, "\n"), - formatted: tsModule.formatDiagnosticsWithColorAndContext(data, formatHost), - category: diagnostic.category, - code: diagnostic.code, - type, - }; - - if (diagnostic.file && diagnostic.start !== undefined) - { - const { line, character } = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start); - entry.fileLine = `${diagnostic.file.fileName}(${line + 1},${character + 1})`; - } - - return entry; - }); -} - export class TsCache { private cacheVersion = "9"; From 2d3cfe32bd1164b6e9c7985f754e657e7c010002 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 30 Aug 2022 16:46:08 -0400 Subject: [PATCH 2/3] test: add unit test for `convertDiagnostic` - this was previously only covered in integration tests - since unit tests don't currently touch `index` or `tscache`, and `convertDiagnostic` was previously in `tscache` - another reason why consolidating these functions into one file made sense --- __tests__/diagnostics.spec.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/__tests__/diagnostics.spec.ts b/__tests__/diagnostics.spec.ts index 141cc8bf..cd57dcdf 100644 --- a/__tests__/diagnostics.spec.ts +++ b/__tests__/diagnostics.spec.ts @@ -4,18 +4,33 @@ import { red } from "colors/safe"; import { makeContext } from "./fixtures/context"; import { setTypescriptModule } from "../src/tsproxy"; -import { printDiagnostics } from "../src/diagnostics"; +import { convertDiagnostic, printDiagnostics } from "../src/diagnostics"; setTypescriptModule(ts); +const tsDiagnostic = { + file: undefined, + start: undefined, + length: undefined, + messageText: "Compiler option 'include' requires a value of type Array.", + category: ts.DiagnosticCategory.Error, + code: 5024, + reportsUnnecessary: undefined, + reportsDeprecated: undefined, +}; + const diagnostic = { flatMessage: "Compiler option 'include' requires a value of type Array.", formatted: "\x1B[91merror\x1B[0m\x1B[90m TS5024: \x1B[0mCompiler option 'include' requires a value of type Array.\n", category: ts.DiagnosticCategory.Error, code: 5024, - type: 'config' + type: "config", }; +test("convertDiagnostic", () => { + expect(convertDiagnostic("config", [tsDiagnostic])).toStrictEqual([diagnostic]); +}); + test("printDiagnostics - categories", () => { const context = makeContext(); From 7c2fdda0fbd4d66280be9490c6565c3222e8760f Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 30 Aug 2022 16:50:14 -0400 Subject: [PATCH 3/3] fix(diagnostics): use `formatHost.getNewLine()` instead of `\n` - since new lines are host OS specific - this fixes the `convertDiagnostic` test on Windows too, where it was failing --- __tests__/diagnostics.spec.ts | 3 ++- src/diagnostics.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/__tests__/diagnostics.spec.ts b/__tests__/diagnostics.spec.ts index cd57dcdf..eb6d0dbf 100644 --- a/__tests__/diagnostics.spec.ts +++ b/__tests__/diagnostics.spec.ts @@ -4,6 +4,7 @@ import { red } from "colors/safe"; import { makeContext } from "./fixtures/context"; import { setTypescriptModule } from "../src/tsproxy"; +import { formatHost } from "../src/diagnostics-format-host"; import { convertDiagnostic, printDiagnostics } from "../src/diagnostics"; setTypescriptModule(ts); @@ -21,7 +22,7 @@ const tsDiagnostic = { const diagnostic = { flatMessage: "Compiler option 'include' requires a value of type Array.", - formatted: "\x1B[91merror\x1B[0m\x1B[90m TS5024: \x1B[0mCompiler option 'include' requires a value of type Array.\n", + formatted: `\x1B[91merror\x1B[0m\x1B[90m TS5024: \x1B[0mCompiler option 'include' requires a value of type Array.${formatHost.getNewLine()}`, category: ts.DiagnosticCategory.Error, code: 5024, type: "config", diff --git a/src/diagnostics.ts b/src/diagnostics.ts index d679f0b1..adc7a8b1 100644 --- a/src/diagnostics.ts +++ b/src/diagnostics.ts @@ -20,7 +20,7 @@ export function convertDiagnostic(type: string, data: tsTypes.Diagnostic[]): IDi return data.map((diagnostic) => { const entry: IDiagnostics = { - flatMessage: tsModule.flattenDiagnosticMessageText(diagnostic.messageText, "\n"), + flatMessage: tsModule.flattenDiagnosticMessageText(diagnostic.messageText, formatHost.getNewLine()), formatted: tsModule.formatDiagnosticsWithColorAndContext(data, formatHost), category: diagnostic.category, code: diagnostic.code,