Declare plugins as peerDependencies

[oliversalzburg]

Is your feature request related to a problem? Please describe.
We use a package manager that utilizes symlinks to build the dependency tree from a flat storage area of all installed packages. Because of that, packages can not require() other packages that live closer to the root of the dependency tree, because their on-disk location doesn't have the required dependency as a parent.

Describe the solution you'd like
npm allows to declare peerDependencies to advertise the desire to a load a module that is not directly depended upon. Declaring a module as a peer dependency ensures that it will be made available to the depending module if it exists in the dependency tree.

Thus, declaring all dependencies moleculer might want to require should be declared as peerDependencies. This will also ensure that it is clear which versions of the dependency moleculer is designed to run with.

Describe alternatives you've considered
Peering can be manually configured in our package manager and that would work just fine for us. I would consider adding the peer dependencies more correct for the reasons given above.

Additional context
It seems some work in this area existed once: #37

[icebob]

Could you explain it with an example?

[oliversalzburg]

In the AMQP transporter, you require amqplib. moleculer does not declare a dependency on amqplib, so this require() will fail, unless a dependent of moleculer installed a version of amqplib into the dependency tree structure into a place where it can be loaded by NodeJS from within the moleculer source code.

The same pattern happens for several other "pluggable" modules in the framework.

This is problematic, because you don't know which version of the required library you will receive and you can't make guarantees regarding compatbilitiy. Thus, the accepted version should be declared in a peerDependencies section in the package.json.

Additionally, there is really no guarantee that the software will be installed in a way that moleculer would be able to require() these modules without declaring them as peers. While the npm and yarn default layouts usually satisfy this requirement, the NodeJS module loader does not dictate this layout in any way.

The real only proper way to resolve this is to have the dependent code pass their instance of the module into your code. Then they can decide which library, with which modifications they get to load and to pass into moleculer. If you want more control in moleculer, declare the dependency in moleculer.

[icebob]

But if I add all available pluggable lib dependencies as peerDependency, but you don't use them, NPM will warn you that these many peer dependency is not installed.

[oliversalzburg]

That is true and it should be seen a benefit IMHO.

It makes it clear that certain features won't be available until the peer dependency is installed or that it will not work at all because of a version conflict.

[shawnmcknight]

This seems to me it would be a better use case for optionalDependencies more than peerDependencies. The expectation of a peerDependency is that both the library's consumer and the library need the dependency (e.g. a library consuming react would also use react-dom, thus react-dom is a peerDependency of react). citation

In the case of moleculer, you have a number of choices as to how you utilize it. For instance, you can choose which transporter to use, which is opt-in functionality. Declaring every transporter in peerDependencies would emit warnings at install for all of the transporters you have no intention of using. Beyond that, the consumer generally wouldn't need to use those dependencies themselves as moleculer abstracts away the transport.

However, optional dependencies might be a good way to express the expected version ranges that moleculer is compatible with.

[icebob]

@shawnmcknight the problem with optional dependencies is that the simple npm install will try installing all of them. So if you make a simple service without transporters, the NPM will install all transporters dependencies as well (nats, ioredis, kafka-node, amqplib...etc) and the node_modules will be too heavy.

[oliversalzburg]

@shawnmcknight I don't necessarily agree with the reasoning, but either way would achieve boths goals I proposed so I don't want to argue over the specifics :D

[ngraef]

As mentioned, npm v6 currently prints a warning if a peer dependency is unmet, which is more of an annoyance than a real problem in this case. That can be solved by marking the peer dependencies as optional in peerDependenciesMeta (supported by npm@6.11, yarn@1.13, pnpm@3.2). However, npm v7 is planning to install peer dependencies by default, so adding all of the possible plugins moleculer accepts would bloat all dependent projects.

I'm going to ping @isaacs here to see if he has any input on the best way to handle this case. How can we avoid bloating dependent projects while still being a good citizen and declaring compatible opt-in dependencies?

[isaacs]

Yeahhh... don't add a zillion things as peer deps, that's going to be a nightmare.

A peer dep is not a thing you might use, but don't load. A peer dep is a thing you do rely on, and need the caller to rely on as well.

So, if I'm understanding the situation properly (which I very well may not be!), the plugins should be declaring moleculer as a peerDependency, not the other way around.

Yes, npm v7 will almost certainly install peer deps by default. Even if not installed, it will fetch their metadata and arrange the tree so that they can be installed without conflicts, or fail if they cannot, so this will still impose a lot of cost if you declare a bunch of them unnecessarily.

Of course the problem is that if moleculer is doing require(somePlugin) to load it, then it won't work with pnpm without using the flag to tell it to flatten everything. (I forget the incantation, it's like --shamelessly-dedupe or some such.)

You could also just document that pnpm users will have to declare their plugins with the full path. Ie, instead of moleculer.registerPluginOrHoweverYouDoIt('some-plugin'), it'd be moleculer.registerPluginOrHoweverYouDoIt(require.resolve('some-plugin')).

For situations like this, where you try to require() something, and then fall back if it's missing, an optionalDependency is the way to go. But again, these are installed by default, so it's not great to have a lot of them that you never even try to load.

Bottom line, you're doing something clever here that leverages an implementation detail of the npm tree reification strategy, and it falls over when using a package manager with the same logical contract but a different implementation strategy.

Looking more closely at what's going on here...

Yeah, I'd probably just pick one of the following:

1. This doesn't work on pnpm without this flag to support unlisted deps, and never will, sorry, that's just how it is. Probably better to try to detect that (maybe look for node_modules/.pnpm/... in the __filename?) so that you can provide a better fallback error message.
2. Provide some way for users to provide the explicit module path from require.resolve() (or the module itself) to be used in the transport. Maybe something like transporters.resolve({ type: 'AMQP', provider: require('amqplib'), options: { other, stuff } })?

[ngraef]

@isaacs Thanks for the input. It's very much appreciated.

To clarify, the "plugins" we're talking about are simply third-party modules that don't know or care about moleculer. Moleculer has built-in adapters for different categories of modules (logger, transporter, serializer, tracer, etc.) and uses a configuration to load the desired pieces at runtime — essentially a factory pattern. It seems like you got that after the "Looking more closely at what's going on here..." bit though. 👍

Based on your knowledge of the ecosystem, do you think there's any room to build in better support for this use case in the package.json dependency structure? Perhaps a new compatibleButNotAutomaticallyInstalledDependencies? (Joking on that name...) If so, that's obviously a separate conversation in the npm org. I'm just curious about your gut feeling.

@icebob and others: It seems like this option is the best way to solve this for now.

2. Provide some way for users to provide the explicit module path from require.resolve() (or the module itself) to be used in the transport. Maybe something like transporters.resolve({ type: 'AMQP', provider: require('amqplib'), options: { other, stuff } })?

This echoes @oliversalzburg's earlier suggestion:

The real only proper way to resolve this is to have the dependent code pass their instance of the module into your code.

[oliversalzburg]

Given that the handling of peer dependencies in package managers seems to have varied in the past and seems to still be unclear, I definitely retrackt the idea of adding peerDeps to moleculer.

Having the ability to pass in the requried code module yourself is always a great option to have. I see no downsides to it, that you don't already have by relying on a package manager.

The worst possible solution is definitely a plugin architecture like babel, ESLint, karma, ... failed to implement properly. Before that is happening, please don't change anything at all :D

[oliversalzburg]

This is really an increasing problem. We're now using yarn2 with PnP and it also checks for dependencies very strictly. Hoisting is not a valid approach to external code loading.

Right now users will have to manually fix these missing dependency links. For example, for yarn2 that would be something like:

packageExtensions:
  "moleculer@0.14.*":
    peerDependencies:
      ioredis: "^4.17.1"
      jaeger-client: "^3.18.0"
      nats: "^1.4.8"

Maybe consider utilizing peerDependenciesMeta to mark them as optional. But an actual solution to this issue would be appreciated.

[isaacs]

As of npm v7, if you mark these as optional peer dependencies, they will not be installed by default. npm v6 and yarn will not warn about them being missing. npm v7 will get upset if they're present, but outside the version range specified (which you probably want).

{
  "peerDependencies": {
    "some-database-driver": "2.x",
    "other-stuff": "1.2.3 - 2.3.4",
    "etc": "...",
  },
  "peerDependenciesMeta": {
    "some-database-driver": {
      "optional": true
    },
    "other-stuff": {
      "optional": true
    },
    "etc": {
      "optional": true
    }
  }
}

It's quite verbose, but I think it will do what you were hoping to accomplish by using peerDependencies in the first place.

[oliversalzburg]

@isaacs Yes, thank you very much. That is far better. I neglected to return to this issue, because I had a workaround in yarn.

I'd be happy to work on a PR for this. @icebob If you agree with the proposal, could you assign this issue to me?