fix(eslint-plugin): [no-var-requires, no-require-imports] support template literal #8408
Conversation
|
Thanks for the PR, @yeonjuan! 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8408 +/- ##
==========================================
+ Coverage 87.14% 87.18% +0.04%
==========================================
Files 251 251
Lines 12272 12279 +7
Branches 3871 3873 +2
==========================================
+ Hits 10694 10706 +12
+ Misses 1306 1304 -2
+ Partials 272 269 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
LGTM, thanks! 🙌
Note: there are other discussions around shared utilities around static things (#8323). Cross-linking there too.
@auvred @kirkwaiblinger you were active in those discussions too. I'd feel bad about not giving you a chance to look at this, if you have time. 😄
| if (node.arguments[0] && isStringOrTemplateLiteral(node.arguments[0])) { | ||
| const argValue = getStaticStringValue(node.arguments[0]); |
There was a problem hiding this comment.
[non-actionable] getStaticStringValue internally checks for node.type === AST_NODE_TYPES.Literal || node.type === AST_NODE_TYPES.TemplateLiteral, so calling isStringOrTemplateLiteral seems a bit redundant here.. Although it returns modified values for non-string literals, but in this particular case (with the require call) I think it is fine, since passing non-string values to the require call would result in a TS error.
I'm fine with the current implementation, just leaving my thoughts here
There was a problem hiding this comment.
Hi @auvred Thanks for sharing your thoughts.
you're right, I added that condition because I was concerned about cases where non-strings(regex, number,..) are used in the arguments of the require() (probably no one writes code like that).
I thought it would be nice if TypeScript Eslint didn't throw an error when non-string value is used and a TS error occurs.
There was a problem hiding this comment.
didn't throw an error when (invalid thing)
Aha, yes, that's a good thing to look out for in general. This is a very good + timely thread! 😄 Thanks - #8549
PR Checklist
Overview