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

Jump to bottom

feat: create no-deprecated-functions #560

Merged
merged 4 commits into from May 4, 2020
Merged

feat: create no-deprecated-functions #560

merged 4 commits into from May 4, 2020

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented May 3, 2020

I've done for the simple route - hopefully jest won't have any big deprecations, but if so we can refactor.

closes #559

/cc @cpojer - this should do as a codemod, tho I'm curious on if Facebook have any x["y"] syntax in your codebase 馃

@G-Rath G-Rath requested a review from SimenB May 3, 2020 10:20
@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch 2 times, most recently from b00f769 to 40c0394 Compare May 3, 2020 10:23
@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch from 40c0394 to 73a5d36 Compare May 3, 2020 10:26
SimenB
SimenB approved these changes May 3, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks! I think we should land and ship this, then make a new major release where this is added to the recommended rules.

@jeysal @thymikee thoughts?

docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
@jeysal
Copy link
Member

jeysal commented May 3, 2020

Haven't reviewed the rule itself, but yeah the concept and it being recommended seems good to me. Also I believe there were a couple of rules I wanted to make recommended last time I worked on eslint-plugin-jest, I hope I documented it in code comments or so, just in case you come across it make sure to enable them as well 馃槄 (on mobile atm)

@SimenB
Copy link
Member

SimenB commented May 3, 2020

Can make https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-jasmine-globals.md recommended as well. Not sure what others you may have wanted 馃榾

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

@SimenB I thought jest.resetModuleRegistry & jest.runTimersToTime were going to be removed in Jest 27, as per this comment.

@SimenB
Copy link
Member

SimenB commented May 3, 2020

Yeah, let's hope so 馃榾

@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch from da89a11 to 0ce9b84 Compare May 3, 2020 11:24
@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

But you suggested documenting them as being scheduled for removal in Jest 26? Was that intentional?

#560 (comment)
#560 (comment)

@SimenB
Copy link
Member

SimenB commented May 3, 2020

Oh... Nope, Jest 27 馃榾

SimenB
SimenB reviewed May 3, 2020
View reviewed changes
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
docs/rules/no-deprecated-functions.md Outdated Show resolved Hide resolved
@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch from 0ce9b84 to 612231a Compare May 3, 2020 11:41
@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch from 612231a to b63c43e Compare May 3, 2020 11:51
@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

Are there plans to ever deprecate the aliases matchers in expect? I know we have no-alias-methods, but we recommend that, so what's the point in the aliases?

@SimenB
Copy link
Member

SimenB commented May 3, 2020

Good point, we could do that. I'm not sure about how much usage they get, might be controversial

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

We can but see 馃し

SimenB
SimenB approved these changes May 3, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Add to main readme and I think we're good to go?

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

Add to main readme

Oh yeah good point - I really should port the readme table generator script I wrote for @typescript-eslint over to here 馃槀

@SimenB
Copy link
Member

SimenB commented May 3, 2020

馃憤 to more automation

thymikee
thymikee approved these changes May 3, 2020
View reviewed changes
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Awesome 馃憤 I'd add it to recommended

cpojer
cpojer approved these changes May 3, 2020
View reviewed changes
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

馃憤 I like it.

I agree this would be fantastic as a codemod. We don't use jest['x'] anywhere.

@SimenB
Copy link
Member

SimenB commented May 3, 2020 •
edited

@G-Rath another one: jestjs/jest#9962. We can probably remove the old one in 27, but let's just say future version in the docs here for now

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020 •
edited

Whens that one going to be removed? nvm you edited your comment :D

@SimenB
Copy link
Member

SimenB commented May 3, 2020

We can probably remove the old one in 27, but let's just say future version in the docs here for now

馃榾

@G-Rath G-Rath force-pushed the create-require-to-jest-rule branch from 207f1e2 to ac8f641 Compare May 3, 2020 20:06
@SimenB
Copy link
Member

SimenB commented May 3, 2020

I edited 2 hours before you commented 馃槈

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 3, 2020

Yes, but you forgot to edit the notification email in my inbox 馃槈

@SimenB SimenB merged commit 55d0504 into master May 4, 2020
@SimenB SimenB deleted the create-require-to-jest-rule branch May 4, 2020 07:49
@SimenB
Copy link
Member

SimenB commented May 4, 2020

Thanks @G-Rath! Thoughts on a new major adding this rule to the recommended set and dropping Node 8?

github-actions bot pushed a commit that referenced this pull request May 4, 2020
@semantic-release-bot
chore(release): 23.9.0 [skip ci]
495c3e9 
# [23.9.0](v23.8.2...v23.9.0) (2020-05-04)

### Features

* create `no-deprecated-functions` ([#560](#560)) ([55d0504](55d0504))
@github-actions
Copy link

github-actions bot commented May 4, 2020

馃帀 This PR is included in version 23.9.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@swissspidy
Copy link

A bit surprised to already get warnings for jest.genMockFromModule even though Jest 26 is not released yet. But I assume that's coming soon :-)

@SimenB
Copy link
Member

SimenB commented May 4, 2020

Ergh, didn't think about that case... Yes, a release is coming today or tomorrow. Might need an option for folks who haven't upgraded yet? I think it makes sense regardless of this issue for people to be able to specify their Jest version, like react: https://github.com/yannickcr/eslint-plugin-react/blob/master/README.md#configuration

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 4, 2020 •
edited

Tracked by #561 - I'll implement it tomorrow morning :) (NZT, so that's ~8-9 hours from now)

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 4, 2020

Also @SimenB I missed your comment re new major + dropping node 8.

I think that'd be a great idea - I think we should land #561 first, & ideally would be nice to have #545 reviewed would cool as that could make sense to include in the recommended as well.

@SimenB
Copy link
Member

SimenB commented May 4, 2020

Thanks @G-Rath!

@SimenB
Copy link
Member

SimenB commented May 4, 2020

We might wanna release a patch skipping genMockFromModule until the option for version is out

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

@cpojer cpojer cpojer approved these changes

@SimenB SimenB SimenB approved these changes

@thymikee thymikee thymikee approved these changes

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

Successfully merging this pull request may close these issues.

new rule: require.requireActual -> jest.requireActual
6 participants
@G-Rath @jeysal @SimenB @swissspidy @cpojer @thymikee