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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(no-deprecated-functions): remove process.cwd from resolve paths #889

Merged
merged 8 commits into from
Sep 29, 2021

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Sep 12, 2021

Resolves #874
Resolves #886
Resolves #888

Would be good if people could test this locally on a few projects and in editors - I think it's fine, but want to make sure we've checked that it won't make any editors explode (as thats the primary reason for why this is here).

@G-Rath G-Rath requested a review from SimenB September 12, 2021 21:14
@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 12, 2021

hmm ok maybe that's why I added it, so we could test properly?

I think we could get almost the best of both worlds by instead doing:

const jestPath = require.resolve('jest/package.json', {
  paths: [
    process.cwd(),
    ...(require.resolve.paths('jest/package.json') ?? []),
  ],
});

But there's one test that still fails.

I'll have to do some thinking and playing around to see what we can do - I'd like to avoid adding a really big e2e-type setup where we copy around a bunch of files if possible but maybe that's what we have to go with?

@SimenB
Copy link
Member

SimenB commented Sep 13, 2021

I think we should just remove it and throw an error if people use this rule without specifying the version of Jest they use. They can easily do this in their own .eslintrc.js via require('jest/package.json').version (or just hard code the version if they want static config in package.json or whatever)

@codejedi365
Copy link

They can easily do this in their own .eslintrc.js via require('jest/package.json').version

Please update the documentation to reflect this concept. It is uncommon to add a settings object to the .eslintrc and I had to go deep into the ESLINT configuration and guess of what to add where.

@SimenB
Copy link
Member

SimenB commented Sep 13, 2021

Our docs are lacking an example of this, yeah

@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 17, 2021

I've made #897 expanding our documentation to explain this :)

@G-Rath G-Rath force-pushed the remove-cmd branch 2 times, most recently from 1158b43 to 60a796e Compare September 17, 2021 22:12
@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 17, 2021

:(

image

@G-Rath G-Rath force-pushed the remove-cmd branch 3 times, most recently from 723cc9e to 2269232 Compare September 18, 2021 00:51
@SimenB
Copy link
Member

SimenB commented Sep 22, 2021

@G-Rath how's this one going? 😀

@G-Rath G-Rath marked this pull request as ready for review September 22, 2021 07:27
@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 22, 2021

@SimenB go for gold on reviewing - there's some minor cleanup I was going to look to do once I got it all green (which it is now) but it's fine without it

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.

this is missing handling of the user passing a string as the version in their settings (via a require('jest/package.json').version)

Beyond that, looks good 🙂

Comment on lines 85 to 88
// pin the original cwd so that we can restore it after each test
// const projectDir = process.cwd();

// afterEach(() => process.chdir(projectDir));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// pin the original cwd so that we can restore it after each test
// const projectDir = process.cwd();
// afterEach(() => process.chdir(projectDir));

Copy link
Collaborator Author

@G-Rath G-Rath Sep 22, 2021

Choose a reason for hiding this comment

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

hehe yeah this was the main "minor cleanup" I was going to do - I think we should be able to use changing the directory to remove the need to pass the projectDir around, which might be nicer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So using process.chdir does work, but tbh I don't know which is actually nicer: using it means we don't have to pass around the project directory argument in all of our tests, but then we'll be magically changing directories and so stuff "just works" 🤔

Copy link
Member

Choose a reason for hiding this comment

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

process.chdir actually changes the dir, so I'd prefer not to

@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 22, 2021

looks good

Glad you think so - I was pleasantly surprised with how nicely this turned out: while a little annoying we can't collect coverage from the process calls, the tests are fast & cleaner than before + we're not making nearly as many tmp directories as before (only ~4 vs "a whole bunch").

At some point I've been meaning to break our utils file up into a few files within utils/, which now that we have this I'll probably do soon.

btw this change should mean we can technically switch to using export = instead of using babel-plugin-replace-ts-export-assignment because that requires there to be only one export which was the case for all but this rule (as it needed to export the _clearJestVersionCache function)

@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 22, 2021

this is missing handling of the user passing a string as the version in their settings (via a require('jest/package.json').version)

Do you mean you'd like a test that explicitly has require('jest/package.json').version? just otherwise I was pretty sure we already had a test in the rule for if the setting is present (since that would otherwise not be covered in coverage), but I'll double check.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 22, 2021

ugh dang @SimenB I see what you mean now about the jest version: require('jest/package.json').version will return the full version, which isn't what we currently expect when it's passed in explicitly. That also means #897 shouldn't be landed as-is as it has the same problem.

const detectJestVersion = (): JestVersion => {
if (cachedJestVersion) {
return cachedJestVersion;
const parseJestVersion = (rawVersion: number | string): JestVersion => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically this is duplicated in detectJestVersion, but I'm fine with that as it's only a few lines and otherwise it calls into question a bit the JestVersion & underlying function name which I don't think is worth doing right now.

It'll be easy enough to refactor in the future if we find more uses for this value and/or a use for having minor or patch versions.

@SimenB SimenB merged commit 6940488 into main Sep 29, 2021
@SimenB SimenB deleted the remove-cmd branch September 29, 2021 07:38
github-actions bot pushed a commit that referenced this pull request Sep 29, 2021
# [24.5.0](v24.4.3...v24.5.0) (2021-09-29)

### Bug Fixes

* **no-deprecated-functions:** remove `process.cwd` from resolve paths ([#889](#889)) ([6940488](6940488))
* **no-identical-title:** always consider `.each` titles unique ([#910](#910)) ([a41a40e](a41a40e))

### Features

* create `prefer-expect-resolves` rule ([#822](#822)) ([2556020](2556020))
* create `prefer-to-be` rule ([#864](#864)) ([3a64aea](3a64aea))
* **require-top-level-describe:** support enforcing max num of describes ([#912](#912)) ([14a2d13](14a2d13))
* **valid-title:** allow custom matcher messages ([#913](#913)) ([ffc9392](ffc9392))
@github-actions
Copy link

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Sep 29, 2021
# [25.0.0-next.5](v25.0.0-next.4...v25.0.0-next.5) (2021-09-29)

### Bug Fixes

* **no-deprecated-functions:** remove `process.cwd` from resolve paths ([#889](#889)) ([6940488](6940488))
* **no-identical-title:** always consider `.each` titles unique ([#910](#910)) ([a41a40e](a41a40e))
* **valid-expect-in-promise:** support `finally` ([#914](#914)) ([9c89855](9c89855))
* **valid-expect-in-promise:** support additional test functions ([#915](#915)) ([4798005](4798005))

### Features

* create `prefer-expect-resolves` rule ([#822](#822)) ([2556020](2556020))
* create `prefer-to-be` rule ([#864](#864)) ([3a64aea](3a64aea))
* **require-top-level-describe:** support enforcing max num of describes ([#912](#912)) ([14a2d13](14a2d13))
* **valid-title:** allow custom matcher messages ([#913](#913)) ([ffc9392](ffc9392))
@github-actions
Copy link

🎉 This PR is included in version 25.0.0-next.5 🎉

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
Projects
None yet
3 participants