-
Notifications
You must be signed in to change notification settings - Fork 149
feat(prefer-wait-for): new rule prefer-wait-for #88
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless it's not too complicated to implement of course 😅
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 👍
Added a few nits for the docs
Having the empty callback with the rest makes sense 👍 |
Co-Authored-By: Tim Deschryver <[email protected]>
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.
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(() => {});` |
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.
💯
lib/rules/prefer-wait-for.js
Outdated
}; | ||
|
||
return { | ||
'CallExpression > Identifier[name=wait]'(node) { |
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.
Unless it's not too complicated to implement of course 😅
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, 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 <[email protected]>
Fix multiline imports and avoid duplicating waitFor import if already present
…to feature/rule-prefer-wait-for
…t-for # Conflicts: # README.md
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.
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 <[email protected]>
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.