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

Add Enzyme deprecation linter to avoid more usage of Enzyme #4346

Closed
wants to merge 1 commit into from
Closed

Add Enzyme deprecation linter to avoid more usage of Enzyme #4346

wants to merge 1 commit into from

Conversation

martijnrusschen
Copy link
Member

Relates to #4300

pullrequest[bot]
pullrequest bot reviewed Oct 30, 2023
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.

Check the status or cancel PullRequest code review here.

pullrequest[bot]
pullrequest bot reviewed Oct 30, 2023
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 219
- 60

98% JavaScript (tests)
2% Other
<1% JSON

Generated lines of change

+ 32
- 0

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

Zarthus
Zarthus approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@Zarthus Zarthus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The actual changes (eslintrc and package.json) look good to me, it seems that at a few places GitHub disagrees with the linter and I think the line has to be moved up one line, but at the same time it's also working fine for other LOCs, it seems like the differentiator is whether it's assigned to a variable by a quick look.

I'm approving it, since the CI is red - once you get it into a green state it'll be good to merge IMO

test/calendar_icon.test.js
@@ -14,6 +14,7 @@ afterAll(() => {
describe("CalendarIcon", () => {
it("renders a custom SVG icon when provided", () => {
const wrapper = mount(
// eslint-disable-line enzyme-deprecation/no-mount
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to move this one line up?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like somehow prettier is moving things to a new line. Not sure if there's a good workaround for this.

Copy link

Choose a reason for hiding this comment

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

Just a guess, but I think it's because you are using eslint-disable-line instead of eslint-disable-next-line. Can you try that instead if you want the comment before the line?

Image of Joey G Joey G

pullrequest[bot]
pullrequest bot reviewed Oct 30, 2023
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

The use of the linter to help with a migration away from enzyme seems like a good idea. I left a few comments around the use of eslint-disable-line vs eslint-disable-next-line that you can try out and see if it fixes the issue brought up by the other reviewer.

Image of Joey G Joey G

Reviewed with ❤️ by PullRequest

test/calendar_test.test.js
@@ -26,7 +26,8 @@
utils.registerLocale("fi", fi);

function getCalendar(extraProps) {
return shallow(
Copy link

Choose a reason for hiding this comment

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

I'm wondering if this is actually doing anything? According to the eslint docs eslint-disable-line goes next to the line it is disabling: https://eslint.org/docs/latest/use/configure/rules#disabling-rules

Same comment for the rest of the changes in this PR.

🔸 Clarify Intent (Important)

Image of Joey G Joey G

pullrequest[bot]
pullrequest bot reviewed Oct 30, 2023
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

No comments beyond the other reviewer. Allowlisting existing callsites via a lint rule seems like a reasonable approach.

Image of Jon Mellman Jon Mellman

Reviewed with ❤️ by PullRequest

pullrequest[bot]
pullrequest bot reviewed Nov 14, 2023
View reviewed changes
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pullrequest pullrequest[bot] pullrequest[bot] left review comments

@Zarthus Zarthus Zarthus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martijnrusschen @Zarthus