Skip to content
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鈥檒l 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

Merged

Conversation

zaicevas
Copy link
Contributor

@zaicevas zaicevas commented May 1, 2021

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? 馃

    await waitFor(() => {
      userEvent.click(button) // reported
      const a = userEvent.click(button) // NOT reported
    })

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a 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/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
@Belco90
Copy link
Member

Belco90 commented May 1, 2021

Also, I've noticed this rule is NOT reporting this case. Is that intentional? 馃


    await waitFor(() => {

      userEvent.click(button) // reported

      const a = userEvent.click(button) // NOT reported

    })

Good catch. It's not intentional, but I detected it when implementing no-unnecessary-act, so it will be addressed there. I'll make sure to include this case in the tests.

@MichaelDeBoey MichaelDeBoey added the enhancement New feature or request label May 1, 2021
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Show resolved Hide resolved
lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
Copy link
Member

@Belco90 Belco90 left a 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/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
Copy link
Member

@Belco90 Belco90 left a 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?

@zaicevas
Copy link
Contributor Author

zaicevas commented May 3, 2021

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)

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a 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

lib/rules/no-wait-for-side-effects.ts Outdated Show resolved Hide resolved
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 馃憡

Copy link
Member

@Belco90 Belco90 left a 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!

@Belco90 Belco90 merged commit 9f5de30 into testing-library:main May 3, 2021
@github-actions
Copy link

github-actions bot commented May 3, 2021

馃帀 This PR is included in version 4.2.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
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make no-wait-for-side-effects report render() inside waitFor
3 participants