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.

Sign up for GitHub

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

Jump to bottom

feat: rename await-fire-event to await-async-event and support user-event #652

Merged
merged 1 commit into from
Oct 2, 2022
Merged

feat: rename await-fire-event to await-async-event and support user-event #652

merged 1 commit into from
Oct 2, 2022

Conversation

skovy
Copy link
Collaborator

@skovy skovy commented Sep 29, 2022 •
edited
Loading

BREAKING CHANGE: rename await-fire-event to await-async-event and support user-event

Checks

  • I have read the contributing guidelines.
  • If some rule is added/updated/removed, I've regenerated the rules list (npm run generate:rules-list)
  • If some rule meta info is changed, I've regenerated the plugin shared configs (npm run generate:configs)

Changes

  • Update the existing await-fire-event rule to become the await-async-event rule
  • Support both user event and fire event via configuration, with the default being user event
  • Update recommended configs to have userEvent for most, but fireEvent for those that have async fire event methods
  • Update related documentation

Context

Resolves #626

Belco90
Belco90 reviewed Sep 29, 2022
View reviewed changes
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.

Thanks for taking care of this! I left some comments.

lib/rules/await-fire-event.ts Outdated
@@ -10,8 +10,13 @@ import {
} from '../node-utils';

export const RULE_NAME = 'await-fire-event';
const EVENT_MODULES = ['fireEvent', 'userEvent'] as const;
Copy link
Member

Choose a reason for hiding this comment

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

We have EVENTS_SIMULATORS exported in utils. Any problem with reusing that one?

lib/rules/await-fire-event.ts Outdated
additionalProperties: false,
properties: {
eventModule: {
type: 'string',
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think I didn't specify this really well in the original ticket. The idea is that this is a list accepting either "fire-event", "user-event", or both!

Frameworks like Vue Testing Library will need to report both, so they'll need to set this option to ["fire-event", "user-event"].

By default, it should be just ["user-event"]

It should be fairly easy to adapt your current code tho.

@Belco90 Belco90 marked this pull request as draft September 29, 2022 13:41
@skovy
Copy link
Collaborator Author

skovy commented Sep 29, 2022

Thanks for the feedback, I'll take a look at those comments! Forgot about draft PRs, thanks for updating that too 🤦‍♂️

@skovy skovy changed the title feat: await-async-event feat: rename await-fire-event to await-async-event and support user-event Sep 30, 2022
@skovy skovy marked this pull request as ready for review September 30, 2022 02:50
Belco90
Belco90 requested changes Sep 30, 2022
View reviewed changes
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.

Great job @skovy! I added a few comments, but should be easy to fix. I think after that it should be ready to go 🚀

README.md Outdated Show resolved Hide resolved
docs/rules/await-async-event.md Outdated Show resolved Hide resolved
docs/rules/await-async-event.md Outdated Show resolved Hide resolved
lib/rules/await-async-event.ts Outdated Show resolved Hide resolved
@skovy skovy mentioned this pull request Oct 1, 2022
Merged
3 tasks
Belco90
Belco90 requested changes Oct 2, 2022
View reviewed changes
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.

Last request! Can you add some tests without setting the option so it takes the default value? This way we explicitly test the default options. Duplicating existing tests just without the option property is fine.

@skovy
feat: await-async-event
2703d5e 
BREAKING CHANGE: rename await-fire-event to await-async-event and add support for user-event
@Belco90 Belco90 merged commit 86c9452 into testing-library:alpha Oct 2, 2022
@skovy skovy deleted the pr/await-async-event branch October 2, 2022 18:56
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

🎉 This PR is included in version 6.0.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MichaelDeBoey MichaelDeBoey added v6 Next major v6 BREAKING CHANGE This change will require a major version bump labels Oct 2, 2022
@MichaelDeBoey MichaelDeBoey linked an issue Oct 4, 2022 that may be closed by this pull request
Transform await-fire-event into await-async-events #626
Closed
MichaelDeBoey pushed a commit that referenced this pull request Oct 4, 2022
@skovy @MichaelDeBoey
feat(await-fire-event): rename to await-async-event + add support f…
b4ce9bb 
…or `user-event` (#652)

BREAKING CHANGE: `await-fire-event` is now called `await-async-event`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Belco90 Belco90 Belco90 approved these changes

Assignees
No one assigned
Labels
BREAKING CHANGE This change will require a major version bump hacktoberfest-accepted released on @alpha v6 Next major v6
Projects
No open projects
eslint-plugin-testing-library v6
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transform await-fire-event into await-async-events
3 participants
@skovy @Belco90 @MichaelDeBoey