feat(prefer-wait-for): new rule prefer-wait-for#88
Conversation
|
should the docs for the other rules related to async utils be updated as well? |
lib/rules/prefer-wait-for.js
Outdated
| }; | ||
|
|
||
| return { | ||
| 'CallExpression > Identifier[name=wait]'(node) { |
There was a problem hiding this comment.
What if the user is using a custom wait util? The rule will report it even if it's not imported from Testing Library?
There was a problem hiding this comment.
Yep. I think it's too much from where it was imported? But I don't know what to do to be honest.
There was a problem hiding this comment.
Obviously it would be better to check where it's imported from, but we have been checking this in other rules without caring about it. Could be an improvement for a separated ticket for all the rules within the plugin?
There was a problem hiding this comment.
On the other hand... It would be interesting to check from where it's imported so we can also replace the import itself. Otherwise, the fix of this rule could leave a broken file.
There was a problem hiding this comment.
I would suggest keep it like this for now and wait for users to ask for it, in case it's a valid use case.
There was a problem hiding this comment.
Unless it's not too complicated to implement of course 😅
timdeschryver
left a comment
There was a problem hiding this comment.
LGTM 👍
Added a few nits for the docs
|
Having the empty callback with the rest makes sense 👍 |
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
emmenko
left a comment
There was a problem hiding this comment.
Looks good overall to me, thanks
| - `waitForElement` | ||
| - `waitForDomChange` | ||
|
|
||
| > This rule will auto fix deprecated async utils for you, including the necessary empty callback for `waitFor`. This means `wait();` will be replaced with `waitFor(() => {});` |
lib/rules/prefer-wait-for.js
Outdated
| }; | ||
|
|
||
| return { | ||
| 'CallExpression > Identifier[name=wait]'(node) { |
There was a problem hiding this comment.
Unless it's not too complicated to implement of course 😅
timdeschryver
left a comment
There was a problem hiding this comment.
LGTM, I have added one comment
I think most of the cases won't need this extra check tho
Fix multiline imports and avoid duplicating waitFor import if already present
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Fix multiline imports and avoid duplicating waitFor import if already present
…to feature/rule-prefer-wait-for
…t-for # Conflicts: # README.md
timdeschryver
left a comment
There was a problem hiding this comment.
I like how you did the import thing 👍
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 <28659384+timdeschryver@users.noreply.github.com>
This closes #86
@timdeschryver let me know what you think! I had to include the empty callback fixer within the the other selectors fixers as otherwise tests were not passing. It's weird tho, the code was executed but apparently not taken into account for the test result.