APIDevTools / swagger-parser

Swagger 2.0 and OpenAPI 3.0 parser/validator
https://apitools.dev/swagger-parser
MIT License
1.1k stars 155 forks source link

swagger-parser 10.1.0 is broken when used with npm < 7 or using --legacy-peer-deps #219

Closed alasdairhurst closed 2 years ago

alasdairhurst commented 2 years ago

One of the changes in last Friday's release broke our CI. looking into it further it's related to how the dependency on AJV@8 gets resolved.

The following error gets thrown when swagger-parser gets required:

Error: Cannot find module 'ajv/dist/core

This is because the dependency resolutions are totally screwed up:

image

In my case, ajv@6 is a transitive dependency in a few other places in my node_modules tree, and npm thought it was a good idea to dedupe ajv@6 where ajv@8 was an (optional) peer dependency in ajv-draft-4. When this gets required by swagger-parser, it then tries to require ajv and fails, even though it's immediate parent swagger-parser has a different (explicit) ajv version...

To be honest, I took a look, and don't see any real issue with the way you've configured your dependencies. This seems like entirely an annoyance with how NPM resolves things, but i'm tracking it here because it was working before the 10.1.0 release came out and added ajv@8 to the mix.

In the meantime i'l pin the version of swagger-parser to prevent users of my library from hitting the issue since i can't force them to use newer versions of npm without a breaking change.

alasdairhurst commented 2 years ago

While it's 100% an NPM resolution issue, It's probably entirely "My problem" due to it being related to my project's specific dependency graph. Looks like you didn't even update ajv, just added it as a dependency and it ended up changing the dependency graph in a way that caused the problem. Huge PITA.

The options are essentially updating all dependencies to use ajv 8 so that it gets deduped instead of @6, or pinning swagger-parser which isn't good.

I wonder if ajv-draft-4 marking the ajv peer dependency as optional is causing npm to behave in this way.

Will close, but hopefully this issue being around/searchable is enough to point someone in the right direction somewhere.

julianlam commented 2 years ago

Is it your problem? It's also broken on my CI as well: https://github.com/NodeBB/NodeBB/runs/6435709547?check_suite_focus=true

Broken on Node v14 only. Newer versions fine.

alasdairhurst commented 2 years ago

@julianlam Sorry, by "my problem" I mean "not necesarrily swagger-parser's problem".

From my limited research, it's related to the npm version. Node 14 doesn't come with npm 7/8 by default and these versions have the change to peer dependencies being installed by default (and some other resolution changes).

If your project is a site that you have full control over then it's possibly simpler to update NPM. Otherwise, if you have something like a library, then it's a lot harder to solve aside from pinning swagger-parser. Espeially if you can't rely on a package-lock or shrinkwrap.

SimeonC commented 2 years ago

We just came across this problem as well. It seems like this should be a swagger-parser issue as from the ajv docs, ajv-draft-04 is not compatible with ajv>=7 so the dependencies as defined in the package.json aren't valid according to ajv (hence why npm complains). https://ajv.js.org/guide/schema-language.html#draft-04

alasdairhurst commented 2 years ago

@SimeonC the docs you're pointing to are referring to the built in support for Json schema draft 4. Since it was removed from ajv, if you rely on draft 4 and use newer versions >=7 of ajv, you need to depend on the separate package (ajv-draft-04).

https://github.com/ajv-validator/ajv-draft-04/blob/2c3f50e9fbaf9e4b2bd7a86d84dc0575a5687a60/package.json#L41

SimeonC commented 2 years ago

Oh, oops. My bad, I completely misread that šŸ˜“. In that case, it is completely an issue with npm (sorry for the excess notifications). Pinning swagger-parser to 10.0.3 worked for us as well.

alasdairhurst commented 2 years ago

No worries, happens to all of us šŸ˜