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

Pin: table@4.0.2, because 4.0.3 needs "ajv": "^6.0.1", causing a conf… #10022

Merged
merged 1 commit into from Feb 28, 2018

Conversation

MS-elug
Copy link
Contributor

@MS-elug MS-elug commented Feb 26, 2018

…lict with eslint 4.18.1 that requires "ajv": "^5.3.0"

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Update package json to avoid peerDependency issue due to the latest release of "table"

What changes did you make? (Give an overview)
Pin: table@4.0.2, because 4.0.3 needs "ajv": "^6.0.1", causing a conflict with eslint 4.18.1 that requires "ajv": "^5.3.0"

Is there anything you'd like reviewers to focus on?

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 26, 2018
@jsf-clabot
Copy link

jsf-clabot commented Feb 26, 2018

CLA assistant check
All committers have signed the CLA.

@platinumazure
Copy link
Member

For the rest of the team: I'm still trying to get eslint-canary working properly so I can validate the ajv upgrade I've proposed some time ago in another PR. So this PR could silence the peer dependency warning until my PR is validated and merged.

Besides the peer dependency warning being confusing, I don't think users experience any other harm (but @MS-elug please correct me if I'm wrong).

The different version of "table" dependency is causing a conflict with eslint 4.18.1 that requires "ajv": "^5.3.0"
@realityking
Copy link
Contributor

While the warning is a bit annoying, everything actually works fine.

The problem is that the ajv-keywords package gets lifted up to be at the same level as eslint’s ajv dependency. This causes npm to complain that the peer dependency ajv-keywords has on acc hasn’t been met.

You can trigger the opposit warning after pinning with a project that requires both eslint and ajv 6. It’s an npm limitation.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to wait for more team members to review.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Feb 26, 2018
@MS-elug
Copy link
Contributor Author

MS-elug commented Feb 26, 2018

While the warning is a bit annoying, everything actually works fine.

I think if we go deeper we can find a failing scenario, for this peerDependency issue.
NPM is warning during the installation that the dependency for table is not installed, meaning that some function of table used by eslint may not work properly.
Then depending on the npm command you used, this can be an error, for instance with our CI, we run the command 'npm ls' that list all the dependencies, and 'by magic' npm change the warning to an error making our CI fails:
image

Fixing the version of table to 4.0.2 (that was the latest version available until 3days ago) solves our CI build failure.

@not-an-aardvark
Copy link
Member

I'm confused about the issue. Why is the peerDependency warning appearing now when it wasn't appearing before? Wouldn't this only happen if someone released a package with a breaking change?

Based on #10022 (comment) it seems like this is a bug in npm.

@realityking
Copy link
Contributor

I just did npm install on fresh clone of master.

As I expected I get the unmet peer dependency warning:

npm WARN ajv-keywords@3.1.0 requires a peer of ajv@^6.0.0 but none is installed. You must install peer dependencies yourself.

But when I run node bin/eslint.js -f table Makefile.js I get a nicely formatted table.

This is really just npm reporting an error that isn't really an error. It might still be worth fixing to not confuse downstream users but it's a not the case that something is broken.

@not-an-aardvark
Copy link
Member

This is really just npm reporting an error that isn't really an error.

Right, I'm saying that this should be filed as a bug in npm, because it shouldn't arrange the dependencies in a manner such that this problem occurs.

I'm fine with hacking around the problem in the meantime, but this seems like something that should be fixed in general.

@zckrs
Copy link

zckrs commented Feb 26, 2018

This missing peer dep will block any lock deps throught npm shrinkwrap

@platinumazure
Copy link
Member

@not-an-aardvark

Right, I'm saying that this should be filed as a bug in npm, because it shouldn't arrange the dependencies in a manner such that this problem occurs.

I'm not sure this is the case. Peer dependencies are logical dependency declarations only, and it's up to the top-level project to declare its own dependencies that resolve all required peer dependencies (in theory; in practice, this is a little more difficult since indirect dependencies may specify peer deps that their consuming projects don't fulfill).

Put another way, npm only checks that ajv@^5.3.0 is available to ESLint, and ajv@^6.0.1 is available to table. This cannot be fulfilled with our current package.json. If anything, table (or one of its dependencies) should explicitly depend on ajv to fulfill the peer dependency and avoid the problem upstream.

If this was a dependency issue rather than peer dependency, I would agree this is an npm issue. But in this case, I'm not sure it's an npm issue.

Let me know if I'm missing something.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Feb 26, 2018

If some version table has a peerDep on ajv 6, and eslint uses table directly, and eslint also needs ajv 5, then eslint can't use that version of table, because they're incompatible.

Whether table needs to peer-depend on ajv versus depend on it is separate; but at the moment, that's how it is.

@realityking
Copy link
Contributor

realityking commented Feb 26, 2018

@platinumazure Not quite.

The situation is the following:
eslint depends on table@^4.0.1 and ajv@^5.3.0
table depends on ajv@^6.0.1 and ajv-keywords@^3.0.0
ajv-keywords has a peerDependency on ajv@^6.0.0

npm arranges theses packages in the following way

eslint

  • table@4.0.3
    • ajv@6.2.0
  • ajv@5.5.2
  • ajv-keywords@3.0.0

Because ajv-keywords@3.0.0 is on the same level as ajv@5.5.2, npm shows this warnings as ajv@5.5.2 is not in range for the ajv-keywords peer dependency.

But when table loads ajv it correctly gets ajv@6.2.0 which does work with ajv-keywords@3.0.0

@ljharb table does not have a peer dependency on ajv. One its dependencies, ajv-keywords, does.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Feb 26, 2018

@realityking ah, thanks. then in that case, table is in error, and should indeed have a direct dependency on ajv - otherwise, that peer dep (which is part of its API) is a breaking change.

@realityking
Copy link
Contributor

@ljharb I'm not sure I follow. table does have a direct dependency on ajv. See here https://github.com/gajus/table/blob/bd4d09e0de99bda1f88fa648fca42d0547eb808e/package.json#L8-L9

@ljharb
Copy link
Sponsor Contributor

ljharb commented Feb 26, 2018

:-/ hmm, in that case i have no idea why this is causing an issue for eslint. This does seem more like a bug in npm, provided rm -rf node_modules && npm install && npm ls is still complaining.

@realityking
Copy link
Contributor

@ljharb See the first part #10022 (comment). Because ajv@5.3 and ajv-keywords@3.0 get installed on the same level in the tree the peer dep validation fails. When accessing the modules in code everything is fine.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Feb 26, 2018

Right; because ajv-keywords has a peer dep, that's satisfied by one of table's unhoisted deps, it's incorrect for npm to hoist it above table.

@platinumazure
Copy link
Member

platinumazure commented Feb 26, 2018 via email

@billyjanitsch
Copy link

@ljharb this is a known issue in npm, unfortuantely. The hoisting algorithm doesn't currently consider whether a package's peer dependencies will be satisfied when hoisting it.

There was a quick/dirty fix in npm 5.2.0 but they rolled it back in 5.3.0 because they wanted to fix it properly & the naive fix broke users with questionable dependency workflows.

@ghost
Copy link

ghost commented Feb 27, 2018

Encountered this issue on standard too: standard/standard#1078 (comment)

This was referenced Mar 22, 2018
@mikermcneil
Copy link

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 29, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet