fix(eslint-plugin): [prefer-find] stop throwing type errors when converting symbols to numbers #8390
Conversation
… symbols to numbers
|
Thanks for the PR, @kirkwaiblinger! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| !callee.optional && | ||
| isStaticMemberAccessOfValue(callee, 'at', globalScope) | ||
| ) { | ||
| const callee = node.callee; |
There was a problem hiding this comment.
This change just reverses the order of whether .at, or the (0), are inspected first. It's not needed for the fix, but I found it surprising that foo(Symbol.for('throws')) would throw, since it's not even an at call; it's just any old function. WIth this change, you have to actually have
x.at(Symbol.for('throws')) in order to trigger the faulty code.
|
|
||
| try { | ||
| asNumber = Number(value); | ||
| } catch (e) { |
There was a problem hiding this comment.
this is all that's strictly necessary for the fix
| const asNumber = Number(value); | ||
| let asNumber: number; | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Refactor] I'm not a big fan of try/catch as a form of program control flow. It somewhat hides the intent of the code. Thrown errors are generally a sign of something truly breaking - IMO it's better to be intentional around code always working as expected.
Instead of generally catching all errors, how about checking if typeof value is 'symbol'?
There was a problem hiding this comment.
yep, I was 50/50 on which way to go on this. Changed!
|
Btw, thanks for the fast followup! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8390 +/- ##
==========================================
- Coverage 87.84% 87.05% -0.79%
==========================================
Files 396 250 -146
Lines 13809 12235 -1574
Branches 4064 3859 -205
==========================================
- Hits 12130 10651 -1479
+ Misses 1382 1317 -65
+ Partials 297 267 -30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Lint failure is unrelated. #8373 |
PR Checklist
Overview
Adds exception handling around Number conversion.