fix(deps): pino-pretty should be an optional peer

[zkochan]

This fixes pnpm/pnpm#1791

pnpm, yarn and npm (from v6.11) are not printing warnings if optional peers are not found.

[jsumners]

Maybe I don't remember how peerDependencies work, but don't they get installed automatically? Our docs are rather clear that you should install pino-pretty yourself if you want to use prettification:

pino/docs/api.md

Lines 183 to 198 in 146d803

	#### `prettyPrint` (Boolean | Object)
	Default: `false`
	Enables pretty printing log logs. This is intended for non-production
	configurations. This may be set to a configuration object as outlined in the
	[`pino-pretty` documentation](https://github.com/pinojs/pino-pretty).
	The options object may additionally contain a `prettifier` property to define
	which prettifier module to use. When not present, `prettifier` defaults to
	`'pino-pretty'`. Regardless of the value, the specified prettifier module
	must be installed as a separate dependency:
	```sh
	npm install pino-pretty
	```

[zkochan]

As of now, peer dependencies are not installed automatically. Unfortunately npm wants to change it in v7. But even then, optional peer deps will not autoinstall

…

On Wed, 21 Aug 2019, 22:39 James Sumners, ***@***.***> wrote: Maybe I don't remember how peerDependencies work, but don't they get installed automatically? Our docs are rather clear that you should install pino-pretty yourself if you want to use prettification: https://github.com/pinojs/pino/blob/146d803e6956638861f57d4d46b275aa8c80fa1d/docs/api.md#L183-L198 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#699?email_source=notifications&email_token=AAOWTG5RVP2DGQ2LJKREPZDQFWKXNA5CNFSM4IOMVE72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD424OXQ#issuecomment-523618142>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOWTGYB72HUXE5JVSFY5F3QFWKXNANCNFSM4IOMVE7Q> .

[jsumners]

I’m not convinced this is a bug. We have the documentation outlining the requirement and issue a warning when you attempt to use it without having installed the dependency.

[zkochan]

But this is clearly an optional peer dependency. It is required by name from the code. What are the drawbacks of declaring it as an optional peer dep if package managers will not print a warning about it? A warning will only be printed if pino-pretty will not much the spec.

Another solution would be changing this line

https://github.com/pinojs/pino/blob/master/lib/tools.js#L159

to

var prettyFactory = require(require.resolve('pino-pretty', {paths: [require.main.filename]}))

[jsumners]

If I have:

package.json

{"dependencies":{"pino":"^5.13.2","pino-pretty":"^3.2.1"}}

Then do pnpm i and run the following script:

'use strict'

const log = require('pino')({prettyPrint: true})
log.info('hello world')

I get:

% node index.js                                                                                                                               
[1566428426105] INFO  (45556 on morla): hello world

Which is to say the require('pino-pretty') resolved the module location and loaded it correctly.

[jsumners]

The reason it isn't a peer dependency is because it is not specifically required for the functionality of pino. It also, theoretically, isn't the only such module that can fill the dependency.

@mcollina @davidmarkclements what are your thoughts?

[mcollina]

The intent of this code is described in #699 (comment), I’ll be +1 on such a change.

peerDependencies are currently a no-no because of how npm currently supports them. I do not want a warning printed during install that the module is missing.

Note that I use this pattern in some other module to work around these limitations (fastify-plugin for example), may I tag you?

[zkochan]

Which is to say the require('pino-pretty') resolved the module location and loaded it correctly.

Currently, this is only an issue in monorepos that are managed by pnpm. pnpm is strict but normally all subdeps have access to the direct dependencies of a project. In a monorepo, pnpm is even stricter and packages have access only to listed deps and peer deps.

peerDependencies are currently a no-no because of how npm currently supports them. I do not want a warning printed during install that the module is missing.

I wanted to write: "The current latest version of npm (v6.11) is not printing warnings for optional peer deps.". But seems like the change by Maël did not work (npm/cli#224), so it still prints a warning. Which is a bug in npm.

The intent of this code is described in #699 (comment), I’ll be +1 on such a change.

I am fine with that solution too. We can close this PR and do the other change.

Note that I use this pattern in some other module to work around these limitations (fastify-plugin for example), may I tag you?

sure

[eps1lon]

peerDependencies are currently a no-no because of how npm currently supports them. I do not want a warning printed during install that the module is missing.

As of npm@6.11 you can use peerDependenciesMeta (this is a yarn RFC but also implemented in npm) to mark peers as optional. This should suppress the warning.

[jsumners]

@eps1lon unless it comes with Node when I do n <release> then I don't have whatever version of npm supports whatever thing. I guarantee I'm not the only one out there that does not care about following along with every latest npm release. So relying on specific versions of npm to resolve the core issue of "we don't want a warning" is not a solution.

[eps1lon]

@jsumners I wasn't suggesting to follow the latest npm install. Just wanted to let you know that this will be resolve and there's no need to be that adamant ("no-no") about peer dependencies. Especially considering not listing those always allowed broken dependency trees.

[mcollina]

I think @jsumners and myself are referring to what is possible right now vs what will be possible in 1-2 years as old node versions go out of LTS.

[davidmarkclements]

I think the reason that npm (and others) have failed to address peer dependencies in a satisfactory way is because there are a multitude of dependency scenarios that we can categorize as peer dependency situations. There is no satisfactory generic approach to peer dependencies.

So considering our specific situation:

1. pino-pretty is for development usage only
2. pino-pretty is only the default prettifier, it can be swapped out for another prettifier such as pino-colada
3. pino-pretty doesn't even have to be used in-process - it can be piped to
4. it's strongly recommended against using it production
5. not everyone wants prettification (I for one prefer seeing the raw JSON, a format designed for human readability)
6. warning messages during install are either ignored (because they're part of noise) or are sometimes acted upon, sometimes by opening issues unnecessarily. We'd like to avoid that
7. given 4., 5. and 6. we want to be able to sign post someone who wants prettification, by informing them at the moment they attempt to use it that a peer dependency is required. Imo, This type of lazy warning has more utility that a contextless warning amongst sponsored messages and what not.
8. If a package json peer dependency does not print a warning message, then it essentially has no utility. The peerDependenciesMeta object cancels out the presence of the peer dependency, except to facilitate some kind of --install-optional-peer-deps flag for the package manager. But if you know to do that, then you also know to install pino-pretty. We only have the one peer dep, so it's really no big deal.
9. Given 8, why would we make a change that causes extra noise and cognitive overhead for users of older node versions - just to follow an incoming standard that may have future utility but in our current situation and at this current time, has none?

So given that, I'd say let's close this PR for now, and revisit if anything changes in the future with regard to this standard (or other standardized approaches to handling peer dep scenarios)

[briancoit]

Wrongfully raised #716 in an attempt to address this. I think the rationale above makes some sense, except there's some assumptions made. See the pnpm use-case - pnpm actually knows what to do with that optional peer dependency and otherwise will NOT allow the dep to be resolved it unless it's specified somewhere as a dependency (which is pretty sane!), so it's largely irrelevant what a user knows to do. I still find it particularly challenging that of all the vendor packages I'm making use of at the moment are compatible (or at least considered pnpm worthy enough to make required fixes (they really are fixes).

As a general rule, it makes sense that if you import/require something, that it's declared as some form of dependency. I also expect you might be forced to reconsider when Yarn2/Berry/PnP land.

[jsumners]

1. I am a big pnpm fan. It’s my primary package manager.

2. If we want to talk about the require statement in the code, then let’s talk about how a package manager is not part of the require spec — http://www.commonjs.org/specs/modules/1.0/ . The code adheres to the CommonJS spec. It looks for the module, does not find it, and notifies the user. Done.

3. A solution was proposed but a PR has not been submitted — fix(deps): pino-pretty should be an optional peer #699 (comment)

[briancoit]

The code adheres to CommonJS spec, but ultimately you're shipping a package, which should probably describe itself to package managers - pnpm included. Taking the literal view, pino-pretty is an optional dependency, which is not described (or constrained). To be clear, my comment around import/require was highlighting the reference to the dependency rather than anything to do with the spec / exact mechanism - not sure why we're conflating these things. More appropriate wording may be "it makes sense that if you depend on external package code, that the dependency is described somewhere".

[davidmarkclements]

I'm in favour of semantic verbosity but not at the expense of user cognitive overhead.

Until the warnings go away in all actively used package managers I'm against altering package.json in a way that causes noise and unnecessary investigation by users.

As soon as usage levels of package managers that don't emit a warning for this config drop to <5% I'm happy to include it.

In the meantime the programmatic fix described seems fine, happy to accept a PR for that.

[eps1lon]

As soon as usage levels of package managers that don't emit a warning for this config drop to <5% I'm happy to include it.

Where do you get usage numbers including the version of the package manager?

[davidmarkclements]

I do not know.

[mcollina]

A good reference would be that it is supported by all version of npm that are shipped in all Node LTS releases. This is likely years away.

[eps1lon]

This is likely years away.

It' already released in node 12, node 10 is at tops months away, node 8 deprecated this december.

I do not know.

So how can you make the statement that you'll merge it once usage drops if you have now knowledge of the usage?

Edit:

Last time node 10 got an npm update it was less than 6 weeks behind node 12: https://nodejs.org/en/download/releases/

[mcollina]

Let's revisit after Node 8 goes EOL if Node 10.x has npm 6.11 we could potentially bump the major and set that version of Node as minimum requirement. I'm not a fan of that tbh, because I would love our software to run on most developer machines without needing an upgrade.

Note that given #699 (comment), it's actually possible to fix this problem now. As I said, I'm happy to see a PR implementing that.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.