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.

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(all): disallow throwing literals (@typescript-eslint/no-throw-literal) #3086

Merged
merged 6 commits into from Nov 22, 2022

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Nov 21, 2022

Better linting

This PR adds the @typescript-eslint/no-throw-literal ESLint rule, which removes the ability to throw non-Error errors.

throw 'msg'; // ❌ this will be flagged

throw {
  message: 'msg', // ❌ this will also be flagged
}

throw new Error('msg'); // ✅ this is good

Slight changes to tsconfig generation

In Forge, we generate each tsconfig.json per-package in a postinstall script. This means there is no global tsconfig.json for the project (aside from a tsconfig.test.json that we use for tests.)

However, since the @typescript-eslint/no-throw-literal rule requires a config to be specified in parserOptions.project, I moved the base TSConfig JavaScript object that we use to generate each package's TSConfig into its own tsconfig.base.json file in the project's root folder.

No more [object Object] errors

Ref #3084

We used to throw a non-standard error object in the core Make API, which ended up logging a [object Object] error in the CLI. This is fixed by following the new lint rule.

Testing

You can test this case easily on macOS by attempting to build the Squirrel.Windows without mono installed. As you can see, this PR adds a lot more clarity to the exact error message.

Before:

An unhandled rejection has occurred inside Forge:
[object Object]

Electron Forge was terminated. Location:

After:

An unhandled rejection has occurred inside Forge:
Error: spawn mono ENOENT
at ChildProcess._handle.onexit (node:internal/child_process:283:19)
    at onErrorNT (node:internal/child_process:476:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

@erickzhao erickzhao requested a review from a team November 21, 2022 22:11
@erickzhao erickzhao changed the title fix(make): clearer error messages when failing at Make step fix(make): clearer error messages for Maker failures Nov 21, 2022
@erickzhao erickzhao marked this pull request as ready for review November 21, 2022 22:27
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Can we add tests?😅

@malept
Copy link
Member

malept commented Nov 21, 2022

I mostly ask because this seems like a regression...

@erickzhao
Copy link
Member Author

@malept You're right. If I'm reading the code correctly, it seems like it regressed way back in #2520, where we added the .toString() call.

I'm guessing the spike in reports is because we have a larger active userbase now that we've brought Forge under the Electron umbrella.

I can add a simple set of tests for this utility function!

@erickzhao erickzhao changed the title fix(make): clearer error messages for Maker failures fix(all): disallow throwing literals (@typescript-eslint/no-throw-literal) Nov 22, 2022
@@ -7,7 +7,6 @@ function redConsoleError(msg: string) {
process.on('unhandledRejection', (reason: string, promise: Promise<unknown>) => {
redConsoleError('\nAn unhandled rejection has occurred inside Forge:');
redConsoleError(reason.toString().trim());
redConsoleError('\nElectron Forge was terminated. Location:');
Copy link
Member Author

Choose a reason for hiding this comment

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

This was just an unnecessary log line

throw err;
} else {
throw new Error(`An unknown error occured while making for target: ${maker.name}`);
Copy link
Member Author

Choose a reason for hiding this comment

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

typo fix

@erickzhao
Copy link
Member Author

@malept After some consideration, enabling a lint rule to make sure all errors get thrown properly is better for code quality in the future.

The regression itself was caused by us doing bad stuff with errors rather than the .toString() call in terminate.ts itself.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Sounds good to me

@erickzhao erickzhao merged commit 3ede278 into main Nov 22, 2022
@erickzhao erickzhao deleted the what-is-this branch November 22, 2022 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants