Was moving request to peerDependencies the best choice?

[analog-nico]

@lorenwest explained in #137 a drawback of request being a peerDependency:

As to peer dependencies being fragile, if two packages require a peer dependency of the same module, they're going to get the same version. If their package.json files require incompatible peerDependency versions for a specific package, you're out of luck. Pick one to use and throw the other one away.

I'm always leery of packages that use peerDependencies, because I may have to find another package and change a bunch of code when it randomly clashes with an unrelated package with a different version of the same peerDependency.

My understanding is peerDependencies was a poor man's solution to the real solution that the latest npm uses - it always installs dependencies as peers unless there's a version clash.

If you have to have a peerDependency for a module to work, it's usually because the module being depended on wasn't designed to work with different instances in same vm.

Fortunately, request-promise loosely depends on request@^2.34. However, issues will indeed occur once request@3 is released if not all libraries used in a project that depend on request are updated at the same time.

Just for reference I created a package that also requires request as a peerDependency but an older version ^1.9.9. I got this output when installing it into a project with request-promise already installed:

My-Mac:request-tests nico$ npm install git+https://github.com/analog-nico/test-old-peer.git
request-tests@1.0.0 /projects/request/request-tests
├── UNMET PEER DEPENDENCY request@2.74.0
└── test-old-peer@1.0.0  (git+https://github.com/analog-nico/test-old-peer.git#ef6c434096ddc04210ecd62e277096ddce633979)

npm WARN test-old-peer@1.0.0 requires a peer of request@^1.9.9 but none was installed.

Defining request as a regular dependency would resolve this but also reopen multiple issues which requested to move request into peerDependencies in order to have more control over the used version of request:

• Dependence to Request #117 Unfortunately, the exact reasons were not discussed.
• Pin request version #76 To pin the request version in the project.
• Update required "request" dependency #100 To pin the request version in a library used by many projects.

IMHO, all available solutions are not perfect:

• request as a regular dependency with a loose defined version ^2.34: See issues above.
• request as a regular dependency with a pinned version: This alternative is maybe the best solution. I just ignored it until now because request-promise, request-promise-native, request-promise-any would need to be updated every time a new request version is released.
• request as a peer dependency with a loose defined version ^2.34: See @lorenwest 's arguments above.
• request as a peer dependency with a pinned version: Not an option because this would immediately clash with other libs.
• request as a peer dependency with an even looser defined version >=2.34: npm would not complain once request@3 is released but with that request-promise's package.json would loose its power to specify with which request versions it is compatible with. At first it probably won't be compatible with request@3 – we expect big changes – and after request-promise is updated it probably won't be compatible with request@2.

This is a tough one...

[ConAntonakos]

Sorry to bump an old thread, I was having conflicting issues with the latest versions of request (2.81.0) and request-promise (4.2.1). I wasn't able to use the object literal as an argument when passing into rp(...). It worked well with a string URI.

Do we need to specifically save request@2.34.0? Thanks!

Node.js v5.12.0

[analog-nico]

Hi @ConAntonakos , I just reran the CI build with request@2.81.0 with node v4 and v6 and everything works fine – including the tests with object literals. Thus it must be a simple bug or something related to your build or execution environment. Feel free to open another issue with details and a code snippet if you cannot figure out the cause.

[ConAntonakos]

Thanks, @analog-nico! It was weird because the latest version that works for me right now is 2.79.0, but not 2.81.0. I appreciate the help, and I apologize again for the bump.

[analog-nico]

My pleasure @ConAntonakos !

[ConAntonakos]

Ok, figured it out! I added the query key to the uri object, and removed the qs key from the object that is passed to request-promise constructor.

Leaving it here in case it helps anyone else! 😄

Here was my old configuration:

{
  uri: {
    protocol: 'http:',
    hostname: hostname,
    port: port,
    pathname: pathname
  },
  qs: qs,
  json: true, 
  timeout: timeout,
  resolveWithFullResponse: true
}

And the updated one:

const uri = url.format({
  uri: {
    protocol: 'http:',
    hostname: hostname,
    port: port,
    pathname: pathname,
    query: qs
  },
});

{
  uri: uri,
  json: true, 
  timeout: timeout,
  resolveWithFullResponse: true
}

[skeggse]

I've got a project that (by normal npm operations over time) ended up hoisting request-promise (actually request-promise-native) such that it no longer had access to its request peer dependency, despite being installed as part of jsdom. IMO this is a worse pathology of peer dependencies.