Make ajv a peer dependency #15

mdurling · 2021-03-05T16:04:03Z

Making ajv a peer dependency of ajv-formats resolves issue #13 that is causing TypeScript version mismatch errors on every ajv upgrade.

coveralls · 2021-03-05T16:05:06Z

Pull Request Test Coverage Report for Build 624953550

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.377%

Totals
Change from base Build 423983827:	0.0%
Covered Lines:	121
Relevant Lines:	128

💛 - Coveralls

mshima · 2021-03-30T12:56:25Z

This PR broke ajv-formats@^1 support with npm@6 and npm@7 with --legacy-peer-deps.

Reproducible by:

npm install @eslint/eslintrc conf --legacy-peer-deps
node -e "require('conf');"

Should be fixed by setting both:

"dependencies": {
  "ajv": "^7.0.0"
},
"peerDependencies": {
  "ajv": "^7.0.0"
},
"peerDependenciesMeta": {
  "ajv": {
    "optional": true
  }
}

epoberezkin · 2021-03-30T18:45:03Z

so it has to be patched in both v1 and v2 then...

epoberezkin · 2021-03-30T18:48:15Z

@mshima would it not bring back the original problem though?

mshima · 2021-03-30T19:02:02Z

@epoberezkin I don't think so, but not sure about yarn and pnpm.
But currently it is completely broken for npm@6 and npm@7 with --legacy-peer-deps
peerDependencies is ignored, see another reproduction:

mkdir repro
cd repro
npm install ajv-formats --legacy-peer-deps
node -e "require('ajv-formats')"

output:

internal/modules/cjs/loader.js:883
  throw err;
  ^

Error: Cannot find module 'ajv'
Require stack:
- /Users/mshima/repro/node_modules/ajv-formats/dist/limit.js

epoberezkin · 2021-03-31T06:27:11Z

I understand that it is broken with this option, I do not fully understand why it is important to support it with this option.

Why cannot you use it without --legacy-peer-deps option?

epoberezkin · 2021-03-31T06:29:22Z

Also, the example above is not representative of real life use cases - there is no use case when you would be installing ajv-formats without also installing ajv.

epoberezkin · 2021-03-31T06:42:26Z

@mshima I am looking for some additional information and/or additional opinions to decide whether this change should be made.

For example, what is the setup (and how common it is) when running npm without --legacy-peer-deps fails (which is the use case for using it, as far as I understand)? Your examples work without it.

Would it work if you run in with --force (which would report the conflict, but could be better)?

I will create a separate issue.

mshima · 2021-04-02T01:59:34Z

For example, what is the setup (and how common it is) when running npm without --legacy-peer-deps fails (which is the use case for using it, as far as I understand)? Your examples work without it.

Our generated project is --legacy-peer-deps safe for angular, not for react neither vue.

What about npm@6? Which is included in every node lts versions.
Easily reproducible with

npx npm@6 install @eslint/eslintrc conf
node -e "require('conf');"

Same output

internal/modules/cjs/loader.js:883
  throw err;
  ^

Error: Cannot find module 'ajv/dist/compile/context'
Require stack:
- /Users/mshima/aplicacoes/ajv/node_modules/ajv-formats/dist/limit.js

Would it work if you run in with --force (which would report the conflict, but could be better)?

--force had many problems recently. It's great when executing manually.
But our workflow executes npm install automatically, It's safer to use npm@6 behavior (which is well known) than using --force.

You should at least update the package.json engines version to require npm >= 7.8.0 since you don't support npm@6 anymore.

We reverted the conf package to a version that doesn't depends on ajv-format at jhipster/generator-jhipster#14531.
And a new release is planned for soon, this won't be a problem anymore for jhipster.

Thanks for your time.

epoberezkin · 2021-04-02T06:55:04Z

need to test the proposed change.

epoberezkin · 2021-04-02T11:53:26Z

@mshima I've tested and it does look like the original problem does not re-occur after this change - thank you!

@drtyh2o can you please test that it is still working ok with your setup?

It is now released as v1.6.1 and v2.0.2

mdurling · 2021-04-02T12:51:59Z

@drtyh2o can you please test that it is still working ok with your setup?

It is now released as v1.6.1 and v2.0.2

v2.0.2 seems to work fine for me. Thanks.

epoberezkin · 2021-04-02T13:10:53Z

Thank you @drtyh2o.

The problem is that with this change I cannot run ajv tests (at least not locally on Mac - they do run on GitHub CI) - the tests use ajv-formats - it's dev dependency for ajv...

Before the test I need to do npm link && npm link ajv - it is needed to test "standalone code", so it can require("ajv"), as it would be for the users.

npm link ajv fails, with npm link ajv --legacy-peer-deps and with npm link ajv --force the tests fail...

@mshima - any idea how it can be fixed?

mshima · 2021-04-02T13:51:44Z

@epoberezkin dev dependency on ajv should be dropped from ajv-formats, it's a prod dependency already.

Before the test I need to do npm link && npm link ajv - it is needed to test "standalone code", so it can require("ajv"), as it would be for the users.

Some of this can be help:

links can be problematic, it may break the linked package node_modules tree. You may try something like npm link && npm link ajv && cd ../ajv-formats && npm install && cd ../ajv
install from github instead of linking when testing with latest version: npm install ajv-validator/ajv-formats#master
GitHub actions uses npm@6, maybe related?
for cross dependency, maybe you should add a mono-repo like package.json in the parent folder for this.
Just in time for npm 7.8.0 that brings lots of fixes supporting this. https://github.com/npm/cli/releases/tag/v7.8.0

I use some cross dependencies too, but test with released versions (normal tests) or install using GitHub link (option 2 for integration tests testing both sides).

mdurling · 2021-04-02T14:07:05Z

The whole point of peerDependencies is that they’re specifically not declared production dependencies. The typical pattern for libraries and plugins is to define these sorts of packages as peerDependencies (for when the library is being installed) and devDependencies (for when the library is being developed). Having ajv be a production dependency of ajv-formats is what causes the original issue whenever new versions ofajv are published. Whoever is installing ajv-formats should be responsible for also installing ajv. Having ajv-formats install a version of ajv is the problem this PR attempts to resolve. While the latest version seems to work, I feel this is destined to break on the next ajv update. My recommendation is to leave ajv in devDependencies and remove it from dependencies.

mshima · 2021-04-02T15:00:13Z

The whole point of peerDependencies is that they’re specifically not declared production dependencies.

That's not true.
https://classic.yarnpkg.com/en/docs/dependency-types/#toc-peerdependencies
This is useful for packages like react that need to have a single copy of react-dom that is also used by the person installing it.
Having single copy of a package so the api is compatible for the whole dependency tree.

peerDependency takes precedency (at least for npm@7), so the dependencies entry is just ignored.
The fix I've proposed is commonly used to fix this kind of problem.
IMO packages should keep npm@6 compatibility at least while node 14 (which includes npm@6) is supported.

Installing with --legacy-peer-deps:

$ npm explain ajv ajv-formats

ajv@6.12.6
node_modules/ajv
  ajv@"^6.12.4" from @eslint/eslintrc@0.4.0
  node_modules/@eslint/eslintrc
    @eslint/eslintrc@"^0.4.0" from the root project

ajv@7.2.4
node_modules/ajv-formats/node_modules/ajv         <--- only installed with `ajv-formats@1.6.1`

ajv@7.2.4
node_modules/conf/node_modules/ajv
  ajv@"^7.0.3" from conf@9.0.2
  node_modules/conf
    conf@"^9.0.2" from the root project

ajv-formats@1.6.1
node_modules/ajv-formats
  ajv-formats@"^1.5.1" from conf@9.0.2
  node_modules/conf
    conf@"^9.0.2" from the root project

Using npm@6 or npm@7 with --legacy-peer-deps, ajv@6 is deduped side by side with ajv-formats@1, breaking ajv-formats.

Using npm@7 without --legacy-peer-deps dedupes correctly:

$ npm explain ajv ajv-formats

ajv@6.12.6
node_modules/ajv
  ajv@"^6.12.4" from @eslint/eslintrc@0.4.0
  node_modules/@eslint/eslintrc
    @eslint/eslintrc@"^0.4.0" from the root project

ajv@7.2.4
node_modules/conf/node_modules/ajv
  ajv@"^7.0.3" from conf@9.0.2
  node_modules/conf
    conf@"^9.0.2" from the root project
  peerOptional ajv@"^7.0.0" from ajv-formats@1.6.1
  node_modules/conf/node_modules/ajv-formats
    ajv-formats@"^1.5.1" from conf@9.0.2
    node_modules/conf
      conf@"^9.0.2" from the root project

ajv-formats@1.6.1
node_modules/conf/node_modules/ajv-formats
  ajv-formats@"^1.5.1" from conf@9.0.2
  node_modules/conf
    conf@"^9.0.2" from the root project

epoberezkin · 2021-04-02T18:51:48Z

While the latest version seems to work, I feel this is destined to break on the next ajv update

I actually did test the scenario of the update, by first having ajv: 7.2.2 in package.json, and then changing it to ajv: ^7.2.4 and running upgrade - it did not break (same for v8).

Also, for unrelated reason, I've published ajv 8.0.4 and then 8.0.5 - did it break the dependency in ajv-format in your case @drtyh2o? I was hoping you'd be able to test the update scenario in a similar way... We can just wait till the next update and see if something breaks.

epoberezkin · 2021-04-02T19:05:08Z

@mshima thanks for the suggestions

dev dependency on ajv should be dropped from ajv-formats, it's a prod dependency already.

indeed, but I don't think it's what causes the issue

GitHub actions uses npm@6, maybe related?

most likely - I was going to add printing version to the test...

install from github instead of linking when testing with latest version

it didn't change anything - it still fails on npm link ajv...

You may try something like npm link && npm link ajv && cd ../ajv-formats && npm install && cd ../ajv

I don't understand - ajv-formats is in node-modules, I am linking npm link ajv inside ajv, not in ajv-formats

Anyway, for now I've downgraded local npm to v6.

Make ajv a peer dependency

50af635

epoberezkin approved these changes Mar 27, 2021

View reviewed changes

epoberezkin merged commit 4baa137 into ajv-validator:master Mar 27, 2021

mshima mentioned this pull request Mar 30, 2021

ERROR! Cannot find module 'ajv/dist/compile/context' when executing jhipster jdl jhipster/generator-jhipster#14492

Closed

epoberezkin mentioned this pull request Mar 31, 2021

Having Ajv as peer dependency breaks npm install --legacy-peer-deps #23

Closed

mshima mentioned this pull request Mar 31, 2021

Revert conf to 9.0.1 jhipster/generator-jhipster#14531

Merged

5 tasks

epoberezkin added a commit that referenced this pull request Apr 2, 2021

change ajv dependency, #15

e274188

epoberezkin added a commit that referenced this pull request Apr 2, 2021

change ajv dependency, #15

130020d

connorjs mentioned this pull request Feb 6, 2022

Doesnt work with ajv@8.8+ #46

Open

mshima mentioned this pull request Nov 9, 2022

Upgrade to Node 18 jhipster/generator-jhipster#20199

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ajv a peer dependency #15

Make ajv a peer dependency #15

mdurling commented Mar 5, 2021

coveralls commented Mar 5, 2021 •

edited

Loading

mshima commented Mar 30, 2021

epoberezkin commented Mar 30, 2021

epoberezkin commented Mar 30, 2021

mshima commented Mar 30, 2021 •

edited

Loading

epoberezkin commented Mar 31, 2021

epoberezkin commented Mar 31, 2021

epoberezkin commented Mar 31, 2021

mshima commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

mdurling commented Apr 2, 2021

epoberezkin commented Apr 2, 2021 •

edited

Loading

mshima commented Apr 2, 2021

mdurling commented Apr 2, 2021

mshima commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

Make ajv a peer dependency #15

Make ajv a peer dependency #15

Conversation

mdurling commented Mar 5, 2021

coveralls commented Mar 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 624953550

💛 - Coveralls

mshima commented Mar 30, 2021

epoberezkin commented Mar 30, 2021

epoberezkin commented Mar 30, 2021

mshima commented Mar 30, 2021 • edited Loading

epoberezkin commented Mar 31, 2021

epoberezkin commented Mar 31, 2021

epoberezkin commented Mar 31, 2021

mshima commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

mdurling commented Apr 2, 2021

epoberezkin commented Apr 2, 2021 • edited Loading

mshima commented Apr 2, 2021

mdurling commented Apr 2, 2021

mshima commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

epoberezkin commented Apr 2, 2021

coveralls commented Mar 5, 2021 •

edited

Loading

mshima commented Mar 30, 2021 •

edited

Loading

epoberezkin commented Apr 2, 2021 •

edited

Loading