Skip to content

Type error in catch clause#2185

Closed
dapplion wants to merge 2 commits intomasterfrom
dapplion/type-error
Closed

Type error in catch clause#2185
dapplion wants to merge 2 commits intomasterfrom
dapplion/type-error

Conversation

@dapplion
Copy link
Copy Markdown
Contributor

@dapplion dapplion commented Mar 17, 2021

The e arg in } catch (e) { is typed as any effectively ts-ignoring all downstream code that consumes it. Typescript 4.0 allows to type the e as unknown microsoft/TypeScript#39015 which is practically the correct type.

Approach 1: no types at all

} catch (e) {
  handleError(e)
}

Approach 2: cast e to Error
If handleError signature changes you will be alerted

} catch (e: unknown) {
  handleError(e as Error)
}

Approach 3: check for Error
Probably overkill since the likelyhood of an external library throwing a non-Error instance is extremely low

} catch (e: unknown) {
  if (e instanceof Error) {
    handleError(e)
  }
}

eslint rule no-implicit-any-catch will enforce this pattern

This PR is meant to gauge interest from Lodestar contributors to adopt this type strategy with errors, before actually doing all the changes necessary. I've just bumped prettier to 2.1 to support type annotations on catch clauses and run a find and replaces

@github-actions github-actions bot added Api scope-networking All issues related to networking, gossip, and libp2p. labels Mar 17, 2021
@qlty-cloud-legacy
Copy link
Copy Markdown

qlty-cloud-legacy bot commented Mar 17, 2021

Code Climate has analyzed commit 05bee21 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 12

View more on Code Climate.

@wemeetagain
Copy link
Copy Markdown
Member

+1 for approach 2

@wemeetagain
Copy link
Copy Markdown
Member

I think we should move forward w approach 2

@3xtr4t3rr3str14l
Copy link
Copy Markdown
Contributor

i feel like it'd almost be easier to make a new PR and change all the errors rather than try to tackle those merge conflicts? maybe if no one wants to resolve all those conflicts, we put this in an issue and close this?

@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Apr 10, 2021

i feel like it'd almost be easier to make a new PR and change all the errors rather than try to tackle those merge conflicts? maybe if no one wants to resolve all those conflicts, we put this in an issue and close this?

@3xtr4t3rr3str14l I just did a search and replace of } catch (e) { for } catch (e: unknown) {. You could reset hard this branch to current master and re-do the changes on top, then force push. This way we keep the PR and it's conversation

@ColinSchwarz ColinSchwarz marked this pull request as draft April 20, 2021 14:19
@stale
Copy link
Copy Markdown

stale bot commented Jun 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Sep 3, 2021

Closing for #3072

@dapplion dapplion closed this Sep 3, 2021
@dapplion dapplion deleted the dapplion/type-error branch September 3, 2021 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope-networking All issues related to networking, gossip, and libp2p.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants