Closed catloversg closed 2 weeks ago
After discussing on Discord, I made these changes:
@typescript-eslint/restrict-template-expressions
and revert all related changes.@typescript-eslint/no-base-to-string
and @typescript-eslint/restrict-plus-operands
: Generally, these rules are not really useful for our case, but we will disable them line-by-line instead of disabling the rules.error: unknown
to a function that expects a string, we can use func(String(error))
. One notable case is the dialogBoxCreate(String(error));
call in src\Corporation\ui
.console.error()
: pass error
as the second parameter.Edit: I also implemented the usage of cause
when rethrowing the error, as d0sboots suggested. If we change our minds on that, I'll revert those changes later.
After discussing on Discord, I made these changes:
- Disable
@typescript-eslint/restrict-template-expressions
and revert all related changes.@typescript-eslint/no-base-to-string
and@typescript-eslint/restrict-plus-operands
: Generally, these rules are not really useful for our case, but we will disable them line-by-line instead of disabling the rules.- When we pass
error: unknown
to a function that expects a string, we can usefunc(String(error))
. One notable case is thedialogBoxCreate(String(error));
call insrc\Corporation\ui
.console.error()
: passerror
as the second parameter.Edit: I also implemented the usage of
cause
when rethrowing the error, as d0sboots suggested. If we change our minds on that, I'll revert those changes later.
Regarding this one When we pass error: unknown to a function that expects a string, we can use func(String(error)). One notable case is the dialogBoxCreate(String(error)); call in src\Corporation\ui.
I was also confused by why it gets triggered on the caught exceptions so I dug a bit deeper. The string interpolation for ${e}
is completely fine for normal use cases. Interpolation itself is not the problem. The rule gets raised when the type in question does not have a guarantee for having a non-trivial toString/toLocaleString implementation.
Most of our use cases are string | number | Error where we check for unknown types and the object types have already been checked for, which are all legit for the linter. We could safely create a helper to type guard against most of these use cases, especially in regards to console.error, log, rethrow or showing errors on the UI.
The only issue is that narrowing primitives or generic objects from 'unknown' still lacks the toString guarantee, it would have to be explicitly checked for known string-compatible types or stringify them with JSON.stringify.
The reason I am mentioning this is that everywhere that
${String(e)}
solves the problem of 'no-base-to-string' for
${e}
the linter should also complain, and it isn't. I'm not sure where that got disabled, though it may be accidental.
Source: https://typescript-eslint.io/rules/no-base-to-string/
For the context of that decision:
dialogBoxCreate
expects a string in this context (JSX.Element
is irrelevant), so we have to pass a string to it.src\Corporation\ui
always calls dialogBoxCreate
with 2 "patterns":
${err}
err + ""
String(error)
.String(error)
and those old "patterns".
This is a part of the series of PRs for #1730. It mostly deals with simple lint errors.