Skip to content

Commit

Permalink
clean: remove ConsoleContext entirely by using buildStart (#414)
Browse files Browse the repository at this point in the history 
- the only reason that `ConsoleContext` was still used was because the `options` hook doesn't support certain context functions like `this.error` / `this.warn` etc
  - but `buildStart` does! so we can just move pretty much everything into `buildStart` instead
    - didn't move setting `rollupOptions` because that value gets hashed and the value in `buildStart` is different, as it is after all plugins' `options` hooks have ran
    - this way we don't need `ConsoleContext` what-so-ever and so can remove it entirely!
      - as well as `IContext`, its tests, etc

- use `this.error` instead of `throw`ing errors in `parse-tsconfig` as it exists now
  - couldn't in the `options` hook, can in `buildStart`
  - this also ensure we don't have all the `rpt2: ` prefixes ever missing again
    - c.f. ff88951, 0628482
  - refactor `parse-tsconfig.spec` to account for this change

- c.f. Rollup hooks docs: https://rollupjs.org/guide/en/#options, https://rollupjs.org/guide/en/#buildstart
  • Loading branch information
@agilgur5
agilgur5 authored Aug 29, 2022
1 parent 79053fe commit 1e71d50
Show file tree
Hide file tree
Showing 9 changed files with 57 additions and 132 deletions.
46 changes: 1 addition & 45 deletions __tests__/context.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,58 +1,14 @@
import { jest, test, expect } from "@jest/globals";

import { makeContext } from "./fixtures/context";
import { ConsoleContext, RollupContext } from "../src/context";
import { RollupContext } from "../src/context";

(global as any).console = {
warn: jest.fn(),
log: jest.fn(),
info: jest.fn(),
};

test("ConsoleContext", () => {
const proxy = new ConsoleContext(6, "=>");

proxy.warn("test");
expect(console.log).toHaveBeenLastCalledWith("=>test");

proxy.error("test2");
expect(console.log).toHaveBeenLastCalledWith("=>test2");

proxy.info("test3");
expect(console.log).toHaveBeenLastCalledWith("=>test3");

proxy.debug("test4");
expect(console.log).toHaveBeenLastCalledWith("=>test4");

proxy.warn(() => "ftest");
expect(console.log).toHaveBeenLastCalledWith("=>ftest");

proxy.error(() => "ftest2");
expect(console.log).toHaveBeenLastCalledWith("=>ftest2");

proxy.info(() => "ftest3");
expect(console.log).toHaveBeenLastCalledWith("=>ftest3");

proxy.debug(() => "ftest4");
expect(console.log).toHaveBeenLastCalledWith("=>ftest4");
});

test("ConsoleContext 0 verbosity", () => {
const proxy = new ConsoleContext(-100);

proxy.warn("no-test");
expect(console.log).not.toHaveBeenLastCalledWith("no-test");

proxy.info("no-test2");
expect(console.log).not.toHaveBeenLastCalledWith("no-test2");

proxy.debug("no-test3");
expect(console.log).not.toHaveBeenLastCalledWith("no-test3");

proxy.error("no-test4");
expect(console.log).not.toHaveBeenLastCalledWith("no-test4");
});

test("RollupContext", () => {
const innerContext = makeContext();
const context = new RollupContext(5, false, innerContext);
Expand Down
6 changes: 3 additions & 3 deletions __tests__/fixtures/context.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { jest } from "@jest/globals";
import { PluginContext } from "rollup";

import { IContext } from "../../src/context";
import { RollupContext } from "../../src/context";

// if given a function, make sure to call it (for code coverage etc)
function returnText (message: string | (() => string)) {
Expand All @@ -11,11 +11,11 @@ function returnText (message: string | (() => string)) {
return message();
}

export function makeContext(): PluginContext & IContext {
export function makeContext(): PluginContext & RollupContext {
return {
error: jest.fn(returnText),
warn: jest.fn(returnText),
info: jest.fn(returnText),
debug: jest.fn(returnText),
} as unknown as PluginContext & IContext;
} as unknown as PluginContext & RollupContext;
};
38 changes: 27 additions & 11 deletions __tests__/parse-tsconfig.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,41 +11,57 @@ const local = (x: string) => normalize(path.resolve(__dirname, x));
const defaultOpts = makeOptions("", "");

test("parseTsConfig", () => {
expect(() => parseTsConfig(makeContext(), defaultOpts)).not.toThrow();
const context = makeContext();

parseTsConfig(context, defaultOpts);

expect(context.error).not.toBeCalled();
});

test("parseTsConfig - incompatible module", () => {
expect(() => parseTsConfig(makeContext(), {
const context = makeContext();

parseTsConfig(context, {
...defaultOpts,
tsconfigOverride: { compilerOptions: { module: "none" } },
})).toThrow("Incompatible tsconfig option. Module resolves to 'None'. This is incompatible with Rollup, please use");
});

expect(context.error).toHaveBeenLastCalledWith(expect.stringContaining("Incompatible tsconfig option. Module resolves to 'None'. This is incompatible with Rollup, please use"));
});

test("parseTsConfig - tsconfig errors", () => {
const context = makeContext();

// should not throw when the tsconfig is buggy, but should still print an error (below)
expect(() => parseTsConfig(context, {
parseTsConfig(context, {
...defaultOpts,
tsconfigOverride: {
include: "should-be-an-array",
},
})).not.toThrow();
});

expect(context.error).toHaveBeenLastCalledWith(expect.stringContaining("Compiler option 'include' requires a value of type Array"));
});

test("parseTsConfig - failed to open", () => {
expect(() => parseTsConfig(makeContext(), {
const context = makeContext();
const nonExistentTsConfig = "non-existent-tsconfig";

parseTsConfig(context, {
...defaultOpts,
tsconfig: "non-existent-tsconfig",
})).toThrow("rpt2: failed to open 'non-existent-tsconfig'");
tsconfig: nonExistentTsConfig,
})

expect(context.error).toHaveBeenLastCalledWith(expect.stringContaining(`failed to open '${nonExistentTsConfig}`));
});

test("parseTsConfig - failed to parse", () => {
const context = makeContext();
const notTsConfigPath = local("fixtures/options.ts"); // a TS file should fail to parse

expect(() => parseTsConfig(makeContext(), {
parseTsConfig(context, {
...defaultOpts,
tsconfig: notTsConfigPath,
})).toThrow(`rpt2: failed to parse '${notTsConfigPath}'`);
})

expect(context.error).toHaveBeenLastCalledWith(expect.stringContaining(`failed to parse '${notTsConfigPath}'`));
});
50 changes: 2 additions & 48 deletions src/context.ts
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
import { PluginContext } from "rollup";

export interface IContext
{
warn(message: string | (() => string)): void;
error(message: string | (() => string)): void;
info(message: string | (() => string)): void;
debug(message: string | (() => string)): void;
}

export enum VerbosityLevel
{
Error = 0,
Expand All @@ -20,46 +12,8 @@ function getText (message: string | (() => string)): string {
return typeof message === "string" ? message : message();
}

/* tslint:disable:max-classes-per-file -- generally a good rule to follow, but these two classes could basically be one */

/** mainly to be used in options hook, but can be used in other hooks too */
export class ConsoleContext implements IContext
{
constructor(private verbosity: VerbosityLevel, private prefix: string = "")
{
}

public warn(message: string | (() => string)): void
{
if (this.verbosity < VerbosityLevel.Warning)
return;
console.log(`${this.prefix}${getText(message)}`);
}

public error(message: string | (() => string)): void
{
if (this.verbosity < VerbosityLevel.Error)
return;
console.log(`${this.prefix}${getText(message)}`);
}

public info(message: string | (() => string)): void
{
if (this.verbosity < VerbosityLevel.Info)
return;
console.log(`${this.prefix}${getText(message)}`);
}

public debug(message: string | (() => string)): void
{
if (this.verbosity < VerbosityLevel.Debug)
return;
console.log(`${this.prefix}${getText(message)}`);
}
}

/** cannot be used in options hook (which does not have this.warn and this.error), but can be in other hooks */
export class RollupContext implements IContext
export class RollupContext
{
constructor(private verbosity: VerbosityLevel, private bail: boolean, private context: PluginContext, private prefix: string = "")
{
Expand All @@ -72,7 +26,7 @@ export class RollupContext implements IContext
this.context.warn(`${getText(message)}`);
}

public error(message: string | (() => string)): void
public error(message: string | (() => string)): void | never
{
if (this.verbosity < VerbosityLevel.Error)
return;
Expand Down
4 changes: 2 additions & 2 deletions src/get-options-overrides.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { createFilter as createRollupFilter, normalizePath as normalize } from "

import { tsModule } from "./tsproxy";
import { IOptions } from "./ioptions";
import { IContext } from "./context";
import { RollupContext } from "./context";

export function getOptionsOverrides({ useTsconfigDeclarationDir, cacheRoot }: IOptions, preParsedTsconfig?: tsTypes.ParsedCommandLine): tsTypes.CompilerOptions
{
Expand Down Expand Up @@ -51,7 +51,7 @@ function expandIncludeWithDirs(include: string | string[], dirs: string[])
return newDirs;
}

export function createFilter(context: IContext, pluginOptions: IOptions, parsedConfig: tsTypes.ParsedCommandLine)
export function createFilter(context: RollupContext, pluginOptions: IOptions, parsedConfig: tsTypes.ParsedCommandLine)
{
let included = pluginOptions.include;
let excluded = pluginOptions.exclude;
Expand Down
27 changes: 13 additions & 14 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { normalizePath as normalize } from "@rollup/pluginutils";
import { blue, red, yellow, green } from "colors/safe";
import findCacheDir from "find-cache-dir";

import { ConsoleContext, RollupContext, IContext, VerbosityLevel } from "./context";
import { RollupContext, VerbosityLevel } from "./context";
import { LanguageServiceHost } from "./host";
import { TsCache, convertDiagnostic, convertEmitOutput, getAllReferences, ICode } from "./tscache";
import { tsModule, setTypescriptModule } from "./tsproxy";
Expand All @@ -24,7 +24,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
let watchMode = false;
let generateRound = 0;
let rollupOptions: InputOptions;
let context: ConsoleContext;
let context: RollupContext;
let filter: any;
let parsedConfig: tsTypes.ParsedCommandLine;
let tsConfigPath: string | undefined;
Expand Down Expand Up @@ -54,7 +54,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
}));
}

const typecheckFile = (id: string, snapshot: tsTypes.IScriptSnapshot | undefined, tcContext: IContext) =>
const typecheckFile = (id: string, snapshot: tsTypes.IScriptSnapshot | undefined, tcContext: RollupContext) =>
{
if (!snapshot)
return;
Expand Down Expand Up @@ -119,8 +119,13 @@ const typescript: PluginImpl<RPT2Options> = (options) =>

options(config)
{
rollupOptions = {... config};
context = new ConsoleContext(pluginOptions.verbosity, "rpt2: ");
rollupOptions = { ...config };
return config;
},

buildStart()
{
context = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: ");

watchMode = process.env.ROLLUP_WATCH === "true" || !!this.meta.watchMode; // meta.watchMode was added in 2.14.0 to capture watch via Rollup API (i.e. no env var) (c.f. https://github.com/rollup/rollup/blob/master/CHANGELOG.md#2140)
({ parsedTsConfig: parsedConfig, fileName: tsConfigPath } = parseTsConfig(context, pluginOptions));
Expand Down Expand Up @@ -157,8 +162,6 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
if (diagnostics.length > 0)
noErrors = false;
}

return config;
},

watchChange(id)
Expand Down Expand Up @@ -210,8 +213,6 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
if (!filter(id))
return undefined;

const contextWrapper = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: ");

const snapshot = servicesHost.setSnapshot(id, code);

// getting compiled file from cache or from ts
Expand All @@ -223,7 +224,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
{
noErrors = false;
// always checking on fatal errors, even if options.check is set to false
typecheckFile(id, snapshot, contextWrapper);
typecheckFile(id, snapshot, context);
// since no output was generated, aborting compilation
this.error(red(`Emit skipped for '${id}'. See https://github.com/microsoft/TypeScript/issues/49790 for potential reasons why this may occur`));
}
Expand All @@ -233,7 +234,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
});

if (pluginOptions.check)
typecheckFile(id, snapshot, contextWrapper);
typecheckFile(id, snapshot, context);

if (!result)
return undefined;
Expand Down Expand Up @@ -299,8 +300,6 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
});
}

const contextWrapper = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: ");

// type-check missed files as well
parsedConfig.fileNames.forEach((name) =>
{
Expand All @@ -310,7 +309,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>

context.debug(() => `type-checking missed '${key}'`);
const snapshot = servicesHost.getScriptSnapshot(key);
typecheckFile(key, snapshot, contextWrapper);
typecheckFile(key, snapshot, context);
});

buildDone();
Expand Down
10 changes: 5 additions & 5 deletions src/parse-tsconfig.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,19 @@ import { dirname } from "path";
import * as _ from "lodash";

import { tsModule } from "./tsproxy";
import { IContext } from "./context";
import { RollupContext } from "./context";
import { printDiagnostics } from "./print-diagnostics";
import { convertDiagnostic } from "./tscache";
import { getOptionsOverrides } from "./get-options-overrides";
import { IOptions } from "./ioptions";

export function parseTsConfig(context: IContext, pluginOptions: IOptions)
export function parseTsConfig(context: RollupContext, pluginOptions: IOptions)
{
const fileName = tsModule.findConfigFile(pluginOptions.cwd, tsModule.sys.fileExists, pluginOptions.tsconfig);

// if the value was provided, but no file, fail hard
if (pluginOptions.tsconfig !== undefined && !fileName)
throw new Error(`rpt2: failed to open '${pluginOptions.tsconfig}'`);
context.error(`failed to open '${pluginOptions.tsconfig}'`);

let loadedConfig: any = {};
let baseDir = pluginOptions.cwd;
Expand All @@ -29,7 +29,7 @@ export function parseTsConfig(context: IContext, pluginOptions: IOptions)
if (result.error !== undefined)
{
printDiagnostics(context, convertDiagnostic("config", [result.error]), pretty);
throw new Error(`rpt2: failed to parse '${fileName}'`);
context.error(`failed to parse '${fileName}'`);
}

loadedConfig = result.config;
Expand All @@ -46,7 +46,7 @@ export function parseTsConfig(context: IContext, pluginOptions: IOptions)

const module = parsedTsConfig.options.module!;
if (module !== tsModule.ModuleKind.ES2015 && module !== tsModule.ModuleKind.ES2020 && module !== tsModule.ModuleKind.ESNext)
throw new Error(`rpt2: Incompatible tsconfig option. Module resolves to '${tsModule.ModuleKind[module]}'. This is incompatible with Rollup, please use 'module: "ES2015"', 'module: "ES2020"', or 'module: "ESNext"'.`);
context.error(`Incompatible tsconfig option. Module resolves to '${tsModule.ModuleKind[module]}'. This is incompatible with Rollup, please use 'module: "ES2015"', 'module: "ES2020"', or 'module: "ESNext"'.`);

printDiagnostics(context, convertDiagnostic("config", parsedTsConfig.errors), pretty);

Expand Down
4 changes: 2 additions & 2 deletions src/print-diagnostics.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { red, white, yellow } from "colors/safe";

import { tsModule } from "./tsproxy";
import { IContext } from "./context";
import { RollupContext } from "./context";
import { IDiagnostics } from "./tscache";

export function printDiagnostics(context: IContext, diagnostics: IDiagnostics[], pretty = true): void
export function printDiagnostics(context: RollupContext, diagnostics: IDiagnostics[], pretty = true): void
{
diagnostics.forEach((diagnostic) =>
{
Expand Down
Loading

0 comments on commit 1e71d50

Please sign in to comment.