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 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 eslint bot added the triage label Feb 26, 2018
@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Feb 26, 2018

CLA assistant check
All committers have signed the CLA.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Feb 26, 2018

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"
@MS-elug MS-elug force-pushed the MS-elug:bugfix/tablePeerDependency branch from 72d935a to 632df43 Feb 26, 2018
@realityking

This comment has been minimized.

Copy link
Contributor

realityking commented Feb 26, 2018

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 left a comment

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

@platinumazure platinumazure added evaluating chore and removed triage labels Feb 26, 2018
@MS-elug

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Contributor

realityking commented Feb 26, 2018

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

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Feb 26, 2018

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

This comment has been minimized.

Copy link

zckrs commented Feb 26, 2018

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

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
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

This comment has been minimized.

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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Contributor

realityking commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Contributor

realityking commented Feb 26, 2018

@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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Member

platinumazure commented Feb 26, 2018

@billyjanitsch

This comment has been minimized.

Copy link

billyjanitsch commented Feb 27, 2018

@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

This comment has been minimized.

Copy link

ghost commented Feb 27, 2018

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

This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@mikermcneil

This comment has been minimized.

@eslint eslint bot locked and limited conversation to collaborators Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.