feat: support TypeScript syntax in no-array-constructor
#19493
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| * 'Array?.(x);', | ||
| * 'Array?.(9);', |
There was a problem hiding this comment.
Should we allow code with optional chaining to support TypeScript syntax?
There was a problem hiding this comment.
Optional chaining isn't a TypeScript syntax. Since Array?.() is already reported by the rule I think it should be here too, yeah. Filed an issue on the extension rule: typescript-eslint/typescript-eslint#10933
There was a problem hiding this comment.
But right now rule just report Array?.() not Array?.(1) should we keep this behavior or reporting all the code with optional chaining?
There was a problem hiding this comment.
Array?.(1) is not reported because it has exactly one argument. It creates an array of length 1, and not array that contains the number 1 as an element. So Array?.(1) cannot be replaced with [1].
For the same reason Array(foo) isn't reported, since it has one argument, but Array(foo, bar) is.
There was a problem hiding this comment.
Got it, thanks!
| * 'Array?.(x);', | ||
| * 'Array?.(9);', |
There was a problem hiding this comment.
Optional chaining isn't a TypeScript syntax. Since Array?.() is already reported by the rule I think it should be here too, yeah. Filed an issue on the extension rule: typescript-eslint/typescript-eslint#10933
lib/rules/no-array-constructor.js
Outdated
| @@ -81,6 +84,7 @@ module.exports = { | |||
| if ( | |||
| node.callee.type !== "Identifier" || | |||
| node.callee.name !== "Array" || | |||
| sourceCode.getTokenAfter(node.callee)?.value === "<" || | |||
There was a problem hiding this comment.
Is there a reason to dip into tokens rather than using the AST directly? This is what the existing typescript-eslint rule does:
| sourceCode.getTokenAfter(node.callee)?.value === "<" || | |
| node.typeArguments || |
There was a problem hiding this comment.
was using @typescript-eslint/parser from astExplorer and it's nodes wasn't working so did this.
|
@JoshuaKGoldberg reminder: when you start triaging and issue or PR, please move it on the Triage board. In this case, it should move to "Implementing" because there is an open issue related to this PR. |
|
👋 Temporary notice, I'm about to merge #19355 chore: formatted files with Prettier via trunk fmt. We should hold off on merging other PRs till it's done. I'll post back soon to confirm we're clear. Cheers! |
|
We're clear! To get rid of these merge conflicts, you can merge from git merge -X ours main |
|
@JoshuaKGoldberg please check the latest changes to see if your feedback has been addressed. |
snitin315
added
the
accepted
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Updated
no-array-constructorrule to support TypeScript syntax now rule will allow follwing coderight now espree parses these codes and thus rule reports them and gives suggestions which are syntax errors like
Is there anything you'd like reviewers to focus on?
Refs #19173