-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): [no-invalid-void-type] allow union of void and allowInGenericTypeArguments
#1960
feat(eslint-plugin): [no-invalid-void-type] allow union of void and allowInGenericTypeArguments
#1960
Conversation
Thanks for the PR, @sinyovercosy! 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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
Codecov Report
@@ Coverage Diff @@
## master #1960 +/- ##
==========================================
- Coverage 93.93% 93.91% -0.02%
==========================================
Files 172 172
Lines 7828 7843 +15
Branches 2245 2253 +8
==========================================
+ Hits 7353 7366 +13
Misses 217 217
- Partials 258 260 +2
|
packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts
Outdated
Show resolved
Hide resolved
Thanks for making this fix. |
if ( | ||
member.type === AST_NODE_TYPES.TSTypeReference && | ||
member.typeParameters?.type === | ||
AST_NODE_TYPES.TSTypeParameterInstantiation | ||
) { | ||
const sourceCode = context.getSourceCode(); | ||
const fullyQualifiedName = sourceCode | ||
.getText(member.typeName) | ||
.replace(/ /gu, ''); | ||
|
||
return ( | ||
fullyQualifiedName === 'Promise' && | ||
member.typeParameters.params.length === 1 && | ||
member.typeParameters.params[0].type === | ||
AST_NODE_TYPES.TSVoidKeyword | ||
); | ||
} | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.
I think this should follow the same rule as the base check.
i.e. instead of special casing Promise
, it should allow all generic types if allowInGenericTypeArguments: true
, or just the whitelisted ones if allowInGenericTypeArguments: string[]
.
I ran into this issue again today. I believe review feedback has been addressed in this PR; Can this get another review or a merge ? |
allowInGenericTypeArguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for this!
Fixes #1956
Fixes #1946
isValidUnionType
is checked if the parent of aVoidKeyword
is aUnionType
. This function only returns true on a union consisting ofvoid
,never
, or valid generic types parametrized byvoid
, respecting theallowInGenericTypeArguments
option. The function should hopefully be extensible if need be in the future.