-
Notifications
You must be signed in to change notification settings - Fork 150
feat: new rule prefer-presence-queries #98
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
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.
some minor typos and a couple use cases we should probably address, but overall looking good, thank you for doing this!
BREAKING CHANGE: rule `no-get-by-for-checking-element-not-present` removed in favour of `prefer-presence-queries`
Co-Authored-By: Ben Monro <[email protected]>
d9f3757
to
7aeab95
Compare
@benmonro after thinking about it again, I don't think this case should be reported:
This is not so simple to identify as vars containing the dom elements are updated dynamically. Take a look at this example: There is a really simple component with a text and a button. When you click the button, the text disappear. In the test (which you can see it's passing) I'm getting the message with This seems like a vicious circle, so I don't think this rule should check queries values saved in vars for its first implementation as it adds lots of complexity. The current PR is the final implementation then. |
@Belco90 hmm yeah I see what you mean. What if we just check for it's use immediately after the query? Reason I ask is that it gives people a way to work around the rule which is what I'm trying to prevent... |
I'm pretty sure there are others ways to skip this and other rules. There are different ways it could be solved, but I'm not sure is worth the effort for now. Even checking if it's used immediately after the query has lots of variants. Would you report this?
Or this?
As you can see, tons of variants would take a while to cover. This could be discussed after this first approach and release it as a minor release. |
@Belco90 ok fair enough this is definitely better so yeah let's ship it and see what happens |
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 #90
BREAKING CHANGE: previous rule
no-get-by-for-checking-element-not-present
has been replaced by this one.