[BUG] pino and rxjs should be peer dependencies of nestjs-pino

[chrismllr]

[X] I've read the docs of nestjs-pino

[X] I've read the docs of pino

[X] I couldn't find the same open issue of nestjs-pino

What is the current behavior?
When using pnpm, installing using the --prod flag omits dependencies used within the code, namely, the following:

• pino
• rxjs

As these are only defined in devDependencies, they are not installed correctly by pnpm.

at the moment, this can be solved with a pnpm readPackage hook to add the correct dependencies in:

function readPackage (pkg, context) {
  if (pkg.name === 'nestjs-pino') {
    pkg.peerDependencies = {
      ...(pkg.peerDependencies ?? {}),
      'pino': '^7.5.0',
      'rxjs': '^7.2.0',
    };
  }
  
  return pkg;
}

What is the expected behavior?
pino, pino-http, and rxjs are included in "dependencies", so they are included in production installs of the package.

Please provide minimal example repo. Without it this issue will be closed
I will attempt to get this for you soon.

Please mention other relevant information such as Node.js version and Operating System.

pnpm: 7.3.0
node: 14.19.0
nestjs-pino: 2.6.0

Edit: Updated, fix can be achieved by simply adding as peerDependencies and installing those packages within project

[iamolegga]

Sorry, I'm not a user of pnpm. But rxjs is a dependency of nestjs, and pino-http is a peer dependency, that should be installed by end user, and it includes pino. This peer dependency allows control of it's versions by end user. Do you install both nestjs-pino and pino-http as mentioned in the docs?

[chrismllr]

I can't claim to say I understand pnpm's resolution patterns fully, but if I had to guess, the nested node_modules structure which pnpm uses places packages in the places a consumer expects them to be based on the package.json of that lib.

In practice, there are issues resolving any pkg which is imported and used directly by a project's codebase if it is not defined either as a peer dependency or a dependency by that project.

The places I had issues:

• https://github.com/iamolegga/nestjs-pino/blob/master/src/PinoLogger.ts#L3
  • PinoLogger imports 'pino' directly
• https://github.com/iamolegga/nestjs-pino/blob/master/src/LoggerErrorInterceptor.ts#L7
  • LoggerErrorInterceptor imports 'rxjs' directly

This is to say, these same modules have no issue with importing @nestjs/common 🤷

I updated my solution above, pino-http was not an issue, only pino and rxjs. Additionally, they were only required to be added in peerDependencies.

[iamolegga]

So, the solution is to add both pino and rxjs to peer deps, right? So, rxjs version in peer deps should be the same as used in nestjs's minimal supported version. And the same for pino, which is used by pino-http. I believe both of them should be set as a range, right? As this package depends on @nestjs/common ^8.0.0 and pino-http ^6.4.0 || ^7.0.0 If you want to fix that feel free to open PR, but please add the links for me to understand that versions are set properly for both packages.

[francoisjacques]

Since this library is to log nestjs log errors through pino, it seems reasonable that pino be exposed as a peerDependency instead of a devDependency like currently.

The workaround works great but really adds to the friction of your library. My two cents.