Skip to content

New rule: no-wait-for-empty-cb #92

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

Closed
Belco90 opened this issue Mar 18, 2020 · 8 comments · Fixed by #94
Closed

New rule: no-wait-for-empty-cb #92

Belco90 opened this issue Mar 18, 2020 · 8 comments · Fixed by #94
Labels
help wanted Extra attention is needed new rule New rule to be included in the plugin released

Comments

@Belco90
Copy link
Member

Belco90 commented Mar 18, 2020

New waitFor method added in dom-testing-library v7 introduces a new restriction on empty async utils: a callback must be passed as param. Empty callbacks are allowed but not recommended. Quoting changelog itself:

it's recommended to avoid an empty callback and instead insert an assertion in that callback function. This is because otherwise your test is relying on the "next tick of the event loop" before proceeding, and that's not consistent with the philosophy of the library:

Because of that, a new rule called no-wait-for-empty-cb could enforce to pass non-empty callbacks to waitFor.

What would be considered a non-empty callback tho? I would say any of these:

  • empty arrow function (i.e. () => {})
  • empty regular function (i.e. function () {})
  • a var referencing any of previously mentioned
  • a function or var called noop?
  • falsy values?
@Belco90 Belco90 added help wanted Extra attention is needed new rule New rule to be included in the plugin labels Mar 18, 2020
@Belco90
Copy link
Member Author

Belco90 commented Mar 18, 2020

@thomlom @emmenko thoughts?

@Belco90 Belco90 mentioned this issue Mar 18, 2020
6 tasks
@timdeschryver
Copy link
Member

I'll try to implement this one !

@timdeschryver
Copy link
Member

Could we reuse this one? https://github.com/eslint/eslint/blob/master/lib/rules/no-empty-function.js
And just do the check for the wait functions?

@Belco90
Copy link
Member Author

Belco90 commented Mar 19, 2020

Of course! Didn't realize ESLint already has a rule for empty functions! How are you planning to reuse it tho? I don't know if there is a way to extend rules but that would be ideal.

@timdeschryver
Copy link
Member

I was just about to copy paste it 😅
Just fyi, I think you should be able to extend it (like I did but for tslint at ngrx-tslint-rules) but then it will warn on every empty callback, not only for the wait functions

@Belco90
Copy link
Member Author

Belco90 commented Mar 19, 2020

I see. I thought ESLint had a more fancy way of doing that, but that's probably too much magic haha.

@Belco90
Copy link
Member Author

Belco90 commented Mar 22, 2020

This will be released on v3

Belco90 added a commit that referenced this issue Mar 29, 2020
feat(await-async-utils): reflect waitFor changes (#89)
feat: new rule no-wait-for-empty-callback (#94)
feat: new rule prefer-wait-for (#88)
feat: new rule prefer-screen-queries (#99)
BREAKING CHANGE: drop support for node v8. Min version allowed is node v10.12 (#96)
BREAKING CHANGE: rule `no-get-by-for-checking-element-not-present` removed in favor of new rule `prefer-presence-queries` (#98)

Closes #85
Closes #86
Closes #90
Closes #92
Closes #95

Co-authored-by: timdeschryver <[email protected]>
@Belco90
Copy link
Member Author

Belco90 commented Mar 29, 2020

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed new rule New rule to be included in the plugin released
Projects
None yet
2 participants