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

Move Jest and ESLint to peerDependencies #55

Merged
merged 4 commits into from
Oct 4, 2018
Merged

Conversation

rogeliog
Copy link
Member

@rogeliog rogeliog commented Oct 2, 2018

Fixes #54

I kept the Jest version at 21 given that the runner works well with Jest 21 and Jest 23 is only needed for the watch plugin. I added the appropriate error for when the user tries to use the watch plugin with an insufficient version of Jest.

@rogeliog rogeliog requested review from ljharb and SimenB October 2, 2018 16:12
@@ -47,6 +47,10 @@
"prettier": "^1.14.3",
"rimraf": "^2.6.2"
},
"peerDependencies": {
"eslint": "^4.0.0 || ^5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

can you remove eslint from dependencies with this change? Also, remove https://github.com/jest-community/jest-runner-eslint/blob/master/src/utils/getLocalESLint.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I just moved it to devDeps...

I guess https://github.com/jest-community/jest-runner-eslint/blob/master/src/utils/getLocalESLint.js now just becomes a regular require('eslint') right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that's the theory!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be both a dep and a peer dep, or a peer and a dev - in either case it will do what you expect. The difference, though, is that if it’s a dep + peer, then it might not be a breaking change to add the peer - if it’s a peer+dev, then it definitely is breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb thanks, you are great!

Copy link
Member

Choose a reason for hiding this comment

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

when it's in dependencies now, we will always require our own, and never the user's. Which means that this now moves from eslint 4 to eslint 5, which is a breaking change either way.

I suppose we could roll back to eslint v4, but we really should allow people to use eslint v5 with this plugin

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not when it's also in peer dependencies - if the user specifies eslint 4, then we won't install v5, we'll prefer their v4.

@rogeliog rogeliog changed the title Add Jest and ESLint as peerDeps Add Jest and ESLint as peerDependencies Oct 2, 2018
@rogeliog rogeliog changed the title Add Jest and ESLint as peerDependencies Mode Jest and ESLint to peerDependencies Oct 2, 2018
@SimenB SimenB changed the title Mode Jest and ESLint to peerDependencies Move Jest and ESLint to peerDependencies Oct 2, 2018
package.json Outdated
@@ -26,7 +26,6 @@
"chalk": "^2.4.1",
"cosmiconfig": "^5.0.0",
"create-jest-runner": "^0.4.1",
"eslint": "^5.6.0",
"find-up": "^3.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

not in use anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

no, I mean find-up 🙂 That's not used anymore

package.json Outdated
@@ -47,6 +47,10 @@
"prettier": "^1.14.3",
"rimraf": "^2.6.2"
},
"peerDependencies": {
"eslint": "^4.0.0 || ^5.0.0",
"jest": ">=21.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be ^21 || ^22 || ^23 since we cant know if it works with v24 yet.

@rogeliog rogeliog merged commit 160e1bb into master Oct 4, 2018
@rogeliog rogeliog deleted the peer-dependencies branch October 4, 2018 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants