-
Notifications
You must be signed in to change notification settings - Fork 147
feat(no-wait-for-side-effects): report render in waitFor #363
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
feat(no-wait-for-side-effects): report render in waitFor #363
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 small improvements to make the code a bit more readable, but otherwise LGTM! 👊
lib/create-testing-library-rule/detect-testing-library-utils.ts
Outdated
Show resolved
Hide resolved
Good catch. It's not intentional, but I detected it when implementing |
lib/create-testing-library-rule/detect-testing-library-utils.ts
Outdated
Show resolved
Hide resolved
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.
@zaicevas Thanks for your PR, it looks great! Just some small tweaks about helpers receiving null
nodes and I think it's ready to go.
lib/create-testing-library-rule/detect-testing-library-utils.ts
Outdated
Show resolved
Hide resolved
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.
Pretty happy with the implementation and its tests! Last requirement: could you mention render
will be reported in the rule docs, and include it in some invalid example?
Done :) Not sure which link to add in Further Reading, tho (if any) |
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.
Just one small thing and than this one can be merged for me
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! 👊
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.
No more further reading links are needed, with existing ones + rule description is more than enough.
Releasing this as a minor then. We haven't set the final SemVer Policy but this sort of extra error reported would be expected at the Feature release level.
I finally won't adapt this implementation with the new function I added to no-unnecessary-act
since my PR is gonna have tons of changes, but I'll note it to do it in a separate PR (or even you can do it if you feel so!)
Thank you very much for your work here @zaicevas!
🎉 This PR is included in version 4.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #360
We should decide how we want to release this enhancement. Seems like the discussion is centered around:
a) minor version bump without an option
b) minor version bump with an option, which is off by default
Also, I've noticed this rule is NOT reporting this case. Is that intentional? 🤔