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

monorepo: Normalize exception values in error handling #3700

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

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Sep 24, 2024

Implements suggestion in issue #3687

Replacing this pattern:

try {
...
} catch (e: any) {
   ...
}

with

try {
...
} catch (e) {
   if (!(e instanceof Error) {
         e = new Error(e)
       }
  ...
}

Changes to tsconfig and eslint.config:

The following rules were disabled

  • @typescript-eslint/no-redeclare: ['Error']
  • no-ex-assign
  • useUnknownInCatchVariables

@@ -56,7 +56,8 @@ module.exports = {
'error',
{ argsIgnorePattern: '^_', varsIgnorePattern: '^_' },
],
'@typescript-eslint/no-redeclare': ['error'],
'@typescript-eslint/no-redeclare': 0,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have to set this down, when reading on no-redeclare we are not redeclaring anyhting but only reassiging, which is totally norma, right?

I did a quick test re-adding the original rule setup and runnpm run lint from root, still seems to work.

@@ -56,7 +56,8 @@ module.exports = {
'error',
{ argsIgnorePattern: '^_', varsIgnorePattern: '^_' },
],
'@typescript-eslint/no-redeclare': ['error'],
'@typescript-eslint/no-redeclare': 0,
'no-ex-assign': 0,
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this rule no-ex-assign seems out of question (as discussed)

@@ -14,6 +14,7 @@
"strict": true,
"target": "es2020",
"lib": ["ES2020", "DOM"],
"skipLibCheck": true
"skipLibCheck": true,
"useUnknownInCatchVariables": false
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this needs to be the otheway around: we want to have unknown there, atm this now goes to any:

grafik

So and this is the version of when removing the new assignment with e still being any:

grafik

So no underlying of e.message (what we would want. any is just saying: use whatever you want, I do not care! 🙂

With unknown is the more strict version - indicating that for e we just don't (and can't) know the type.

Ah, just seeing that a plain use in new Error() is then not directly possible. 🤔

grafik grafik

But for any we still have typed as any. Also not optimal:

grafik

Hmm. Let me give this some thought, input welcome from the team.

@holgerd77
Copy link
Member

Will give this a "WIP" label again to indicate that we need to re-think this to some extend and this needs some continued work, so that this won't get merged accidentally.

We nevertheless should already think and reason about this.

@holgerd77
Copy link
Member

So this has settled to the following (in discussion with @jochem-brouwer):

  1. Make it unknown with useUnknownInCatchVariables ts config variable (which resembles reality)
  2. Check for Error instance, then do normal error handling
  3. Otherwise re-throw

So that we basically do an adopted version from this StackOverflow answer, so like:

try {
  throw new Error('foo');
} catch (err) { // unknown not necessary since we set the flag
  if (err instanceof Error) {
    // Do what we did before with the error
  } else {
    // Rethrow the error
  }
}

Jochem, could you do a final confirmation (or correction) here?

@ScottyPoi if you can go along + Jochem confirms, can you update the PR? Thanks! 🙏 🙂

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Sep 27, 2024

Yes this is correct! Note, thus set useUnknownInCatchVariables to true!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants