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.

Sign up for GitHub

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

fix(cli): properly handle errors #217

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix(cli): properly handle errors #217

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b10ede
fix(cli): properly handle errors
Jan 2, 2024
5de7137
fix(cli): sass error handling
Jan 2, 2024
cd84c1b
fix(cli): properly wrap and colorize sass errors
Jan 2, 2024
e2b0097
test: fix broken tests
Jan 2, 2024
d939266
fix: handling of error objects
Jan 3, 2024
1d515a5
fix: lint check
Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions __tests__/cli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { execSync } from "child_process";
import { execSync, spawnSync } from "child_process";

describe("cli", () => {
it("should run when no files are found", () => {
const result = execSync("npm run typed-scss-modules src").toString();
const result = spawnSync("npm run typed-scss-modules src", {
shell: true,
});

expect(result).toContain("No files found.");
expect(result.stderr.toString()).toContain("No files found.");
Copy link
Owner

Choose a reason for hiding this comment

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

why these changes?

Copy link
Author

Choose a reason for hiding this comment

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

@skovy this was needed because of the changes to use console.warn where appropriate. NodeJS sends console.warn messages to stderr (appropriately) -- additionally execSync doesn't expose messages in stderr of the downstream process. spawnSync however does let you inspect stdout and stderr

Copy link
Owner

Choose a reason for hiding this comment

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

Got it, that makes sense. While here and making this change, can we make them all spawnSync for consistency? and it sounds like it supports more functionality which we need here so others might need this in the future as well

});

describe("examples", () => {
Expand Down
36 changes: 20 additions & 16 deletions __tests__/core/alerts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ import { alerts, setAlertsLogLevel } from "../../lib/core";

describe("alerts", () => {
let logSpy: jest.SpyInstance;
let warnSpy: jest.SpyInstance;

beforeEach(() => {
logSpy = jest.spyOn(console, "log").mockImplementation();
warnSpy = jest.spyOn(console, "warn").mockImplementation();
});

afterEach(() => {
logSpy.mockRestore();
warnSpy.mockRestore();
});

const TEST_ALERT_MSG = "TEST ALERT MESSAGE";
Expand All @@ -20,74 +23,74 @@ describe("alerts", () => {

alerts.error(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.warn).toHaveBeenLastCalledWith(EXPECTED);
//make sure each alert only calls console.log once
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenCalledTimes(1);

alerts.warn(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(2);
expect(console.warn).toHaveBeenLastCalledWith(EXPECTED);
expect(console.warn).toHaveBeenCalledTimes(2);

alerts.notice(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(3);
expect(console.log).toHaveBeenCalledTimes(1);

alerts.info(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(4);
expect(console.log).toHaveBeenCalledTimes(2);

alerts.success(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(5);
expect(console.log).toHaveBeenCalledTimes(3);
});

it("should only print error messages with error log level", () => {
setAlertsLogLevel("error");

alerts.error(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenLastCalledWith(EXPECTED);
expect(console.warn).toHaveBeenCalledTimes(1);

alerts.warn(TEST_ALERT_MSG);
alerts.notice(TEST_ALERT_MSG);
alerts.info(TEST_ALERT_MSG);
alerts.success(TEST_ALERT_MSG);

//shouldn't change
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenCalledTimes(1);
});

it("should print all but warning messages with info log level", () => {
setAlertsLogLevel("info");

alerts.error(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenLastCalledWith(EXPECTED);
expect(console.warn).toHaveBeenCalledTimes(1);

alerts.notice(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(2);
expect(console.log).toHaveBeenCalledTimes(1);

alerts.info(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(3);
expect(console.log).toHaveBeenCalledTimes(2);

alerts.success(TEST_ALERT_MSG);

expect(console.log).toHaveBeenLastCalledWith(EXPECTED);
expect(console.log).toHaveBeenCalledTimes(4);
expect(console.log).toHaveBeenCalledTimes(3);

alerts.warn(TEST_ALERT_MSG);

expect(console.log).toHaveBeenCalledTimes(4);
expect(console.warn).toHaveBeenCalledTimes(1);
});

it("should print no messages with silent log level", () => {
Expand All @@ -100,5 +103,6 @@ describe("alerts", () => {
alerts.success(TEST_ALERT_MSG);

expect(console.log).not.toHaveBeenCalled();
expect(console.warn).not.toHaveBeenCalled();
});
});
9 changes: 8 additions & 1 deletion __tests__/core/generate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,16 @@ describeAllImplementations((implementation) => {
updateStaleOnly: false,
logLevel: "verbose",
outputFolder: null,
aliases: {
"~fancy-import": "complex",
"~another": "style",
},
aliasPrefixes: {
"~": "nested-styles/",
},
});

expect(fs.writeFileSync).toHaveBeenCalledTimes(6);
expect(fs.writeFileSync).toHaveBeenCalledTimes(9);
Copy link
Author

Choose a reason for hiding this comment

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

This number is higher because more files are now compiled, since the tool forces you to handle build errors. This is also why setting the aliases and aliasPrefix was required for this test.

});
});
});
18 changes: 10 additions & 8 deletions __tests__/core/list-different.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ describeAllImplementations((implementation) => {

beforeEach(() => {
console.log = jest.fn();
console.warn = jest.fn();
exit = jest.spyOn(process, "exit").mockImplementation();
});

Expand Down Expand Up @@ -41,10 +42,10 @@ describeAllImplementations((implementation) => {
});

expect(exit).toHaveBeenCalledWith(1);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`[INVALID TYPES] Check type definitions for`)
);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`invalid.scss`)
);
});
Expand All @@ -69,8 +70,8 @@ describeAllImplementations((implementation) => {
outputFolder: null,
});

expect(console.log).toHaveBeenCalledTimes(1);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`Only 1 file found for`)
);
expect(exit).not.toHaveBeenCalled();
Expand All @@ -96,6 +97,7 @@ describeAllImplementations((implementation) => {
});

expect(exit).not.toHaveBeenCalled();
expect(console.warn).not.toHaveBeenCalled();
expect(console.log).not.toHaveBeenCalled();
});

Expand All @@ -119,12 +121,12 @@ describeAllImplementations((implementation) => {
});

expect(exit).toHaveBeenCalledWith(1);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(
`[INVALID TYPES] Type file needs to be generated for`
)
);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`no-generated.scss`)
);
});
Expand All @@ -149,8 +151,8 @@ describeAllImplementations((implementation) => {
});

expect(exit).not.toHaveBeenCalled();
expect(console.log).toHaveBeenCalledTimes(1);
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`No files found`)
);
});
Expand Down
6 changes: 3 additions & 3 deletions __tests__/core/list-files-and-perform-sanity-check.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ const options: ConfigOptions = {

describe("listAllFilesAndPerformSanityCheck", () => {
beforeEach(() => {
console.log = jest.fn();
console.warn = jest.fn();
});

it("prints a warning if the pattern matches 0 files", () => {
const pattern = `${__dirname}/list-different/test.txt`;

listFilesAndPerformSanityChecks(pattern, options);

expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining("No files found.")
);
});
Expand All @@ -37,7 +37,7 @@ describe("listAllFilesAndPerformSanityCheck", () => {

listFilesAndPerformSanityChecks(pattern, options);

expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining("Only 1 file found for")
);
});
Expand Down
2 changes: 2 additions & 0 deletions __tests__/dummy-styles/global-variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
$global-red: #ff0000;

.globalStyle {
background-color: $global-red;
}
6 changes: 3 additions & 3 deletions __tests__/typescript/class-names-to-type-definitions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ jest.mock("../../lib/prettier/can-resolve", () => ({

describe("classNamesToTypeDefinitions (without Prettier)", () => {
beforeEach(() => {
console.log = jest.fn();
console.warn = jest.fn();
});

describe("named", () => {
Expand Down Expand Up @@ -41,7 +41,7 @@ describe("classNamesToTypeDefinitions (without Prettier)", () => {
});

expect(definition).toEqual("export declare const myClass: string;\n");
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`[SKIPPING] 'if' is a reserved keyword`)
);
});
Expand All @@ -54,7 +54,7 @@ describe("classNamesToTypeDefinitions (without Prettier)", () => {
});

expect(definition).toEqual("export declare const myClass: string;\n");
expect(console.log).toHaveBeenCalledWith(
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(`[SKIPPING] 'invalid-variable' contains dashes`)
);
});
Expand Down
7 changes: 6 additions & 1 deletion lib/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env node

import yargs from "yargs";
import { alerts } from "./core";
import { IMPLEMENTATIONS } from "./implementations";
import { main } from "./main";
import { Aliases, NAME_FORMATS } from "./sass";
Expand Down Expand Up @@ -145,4 +146,8 @@ const { _: patterns, ...rest } = yargs
.parseSync();

// eslint-disable-next-line @typescript-eslint/no-floating-promises
main(patterns[0] as string, { ...rest });
main(patterns[0] as string, { ...rest }).catch((error) => {
alerts.error("Encountered an error while generating type definitions.");
alerts.error(error);
process.exitCode = 1;
});
23 changes: 21 additions & 2 deletions lib/core/alerts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import chalk from "chalk";
import { SassError } from "node-sass";

export const LOG_LEVELS = ["verbose", "error", "info", "silent"] as const;
export type LogLevel = (typeof LOG_LEVELS)[number];
Expand Down Expand Up @@ -32,12 +33,30 @@ const withLogLevelsRestriction =
}
};

const normalizeErrorMessage = (error: string | Error) => {
if (error && error instanceof Error && "file" in error) {
const { message, file, line, column } = error as SassError;
const location = file ? ` (${file}[${line}:${column}])` : "";

const wrappedError = new Error(`SASS Error ${location}\n${message}`, {
cause: error,
});

wrappedError.stack = chalk.red(wrappedError.stack);

return wrappedError;
}

return chalk.red(error);
};
const error = withLogLevelsRestriction(
["verbose", "error", "info"],
(message: string) => console.log(chalk.red(message))
(message: string | Error) => {
console.warn(normalizeErrorMessage(message));
}
);
const warn = withLogLevelsRestriction(["verbose"], (message: string) =>
console.log(chalk.yellowBright(message))
console.warn(chalk.yellowBright(message))
);
const notice = withLogLevelsRestriction(
["verbose", "info"],
Expand Down
Loading