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

expand peerDep to allow eslint@6 #70

Open
wants to merge 2 commits into
base: master
from

Conversation

@kentcdodds
Copy link

commented Aug 7, 2019

I'm using this with ESLint 6 in my project and it works fine 馃憤

@ljharb
Copy link
Collaborator

left a comment

Thanks - actually this project has a bug anyways; it's got eslint as a dep but without a verbatim copy of the peer dep string. Can you update the dep string to match the peer dep string?

@kentcdodds

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

Actually, because it has a peerDep it shouldn't have eslint listed as a dep right?

@@ -26,14 +26,14 @@
"dependencies": {
"chalk": "^2.4.1",
"cosmiconfig": "^5.0.0",
"create-jest-runner": "^0.5.3",
"eslint": "^5.6.0"

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Aug 7, 2019

Author

Having eslint listed in the dependencies resulted in potentially downloading multiple copies of eslint. Let's rely on the peerDep people should be installing themselves.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2019

Collaborator

deduping handles this - it's supposed to be both.

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Aug 7, 2019

Author

No, deduping will not handle this. It will see that this module requires no greater than eslint 5 and my project requires eslint 6 so it will install both.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 8, 2019

Collaborator

oh sure, that's the issue with what's on master - that's why the RHS of "eslint" must match the peer dep :-)

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Aug 8, 2019

Author

Sorry, I'm not sure I understand. So are your saying the PR is good?

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 8, 2019

Collaborator

No - I'm saying that since the peerDep string is "^4.0.0 || ^5.0.0 || ^6.0.0", eslint needs the same exact value in either deps or devDeps.

Putting it in deps means that people don't have to necessarily have eslint installed to invoke it via this runner, which is the goal here. If there's an incompatible eslint installed up the tree, the peer + dep combo will make npm ls fail (which everyone should be always running in CI anyways, since if it fails, your dep graph is invalid and you can't rely on anything working)

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Aug 8, 2019

Author

I see what you mean. It's wired because if someone doesn't have eslint installed they'll get a warning in the terminal, but it's your project, so sure.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 8, 2019

Collaborator

With peer + dep, they won't in fact get a warning - eslint will just be automatically installed and everything will Just Work.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

It should; it's totally fine (and often preferred) to have a peerDep listed also as a dep - regardless, peer deps should always be listed as either peer + dep, or peer + dev dep - never just as a peer dep.

@ljharb

ljharb approved these changes Aug 8, 2019

@ljharb ljharb force-pushed the kentcdodds:patch-1 branch from 61e7eb1 to b5f92a6 Aug 8, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Looks like tests are failing; more test changes may be required here.

@kentcdodds

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

I don't have a lot of bandwidth here 馃槵, but maybe changing the parserOptions from null to {} will fix it.

@SimenB

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

eslint@6 has dropped support for node 6, so that will keep failing until we drop it as well.


I wonder if we should run CI against all versions of eslint we claim to support. Right now we only run against whatever is the latest of them.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

@SimenB we can just exclude the eslint 6 / node 6 combo. We definitely should be testing every eslint and node version we support together.

@SimenB

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Yeah, that makes sense to me. Last time I had a testing matrix like that I gave up on travis and moved to circle ci, but I bet we can make it work now that travis has stages and other more advanced options

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

@SimenB it's pretty easy with travis, see the airbnb config monorepo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.