Skip to content
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

chore: Add @typescript-eslint/no-confusing-void-expression rule #2910

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Nov 25, 2024

Added new eslint rule: https://typescript-eslint.io/rules/no-confusing-void-expression/

This rule disallows e.g. using return foobar() when foobar() method returns void. It is better to just call the method and do explicit return separately, if needed. (Quite typically the return is not needed at all as the line is the last line of the method.)

Simplified multiple-publisher-plugins.test.ts so that it uses Promise<void> instead of Promise<unknown> and therefore doesn't need any special handling related to this rule.

@github-actions github-actions bot added network Related to Network Package test-utils Related to Test Utils Package dht Related to DHT package utils sdk node labels Nov 25, 2024
Copy link
Contributor

@juslesan juslesan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this rule need to be ignored in Promise.all calls and some other places?

@teogeb
Copy link
Contributor Author

teogeb commented Nov 26, 2024

Why does this rule need to be ignored in Promise.all calls and some other places?

It would be better to refactor these tests. In practice, the arguments sent to Promise.all() are evaluated before the Promise.all() call (like we do for all function calls: we need the value for parameters before we can pass those as arguments). Therefore it would be clearer if we'd move void calls out from Promise.all(), i.e. execute the lines explicitly before the Promise.all() is called.

@teogeb teogeb requested a review from juslesan November 26, 2024 12:16
Copy link
Contributor

@juslesan juslesan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could move the void expressions above the Promise.all blocks in all cases to get rid of the eslint-disable comments

@teogeb teogeb merged commit f2a4852 into main Nov 26, 2024
24 checks passed
@teogeb teogeb deleted the jest-add-no-confusing-void-expression branch November 26, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dht Related to DHT package network Related to Network Package node sdk test-utils Related to Test Utils Package utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants