Make this lib peerDepend on loopback-connector

[bobbysmith007]

Description

• Allows sharing object models between this libary and the others
  helping orchestrate this (eg: loopback-connector,
  loopback-connector-postgresql)
• EG: All copies of the library will share one
  loopback-connector.ParameterizedSQL object

see: AccelerationNet/loopback-connector-postgresql@0661c6c

Tests

I was unsure how to add tests for this as the bug is only apparent in a full working loopback, but there are possible bugs because the loopback-connector-postgresql creates ParameterizedSQL objects that
are incompatible with the ones here because both list loopback-connector as a dependency rather than a peerDependency. If we are sharing object models, they all need to come from one instance of the library rather than from separate ones

Related issues

• Add loopback-connector into peer dependencies loopback-connector-postgresql#246

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[ssh24]

@bobbysmith007 Please sign the CLA before I can proceed with the PR: https://cla.strongloop.com/agreements/strongloop/loopback-datasource-juggler

[ssh24]

@slnode test please

[ssh24]

LGTM

[ssh24]

@slnode test please

[raymondfeng]

@bobbysmith007 Are you aware of the complication around peerDependencies - https://docs.npmjs.com/files/package.json#peerdependencies?

[bobbysmith007]

raymondfeng: I read that page before deciding that they were the correct path to go. (Initially I had only wanted to list them in devDependencies)

Its definitely wrong / buggy to try and share object models between the three libraries (relying on instanceof) without them all sharing the same exact references. Peer dependencies looks to be trying to solve this exact problem (pluggable modules that share object models). Also it seems that it should be the applications responsibility to depend on loopback-connector-dbbackend and loopback and it is loopbacks job to depend on loopback-connector and loopback-datasource-juggler, which I think leaves us in a situation where all the libraries will be installed at once without experiencing the warning about libraries being required but not installed. Additionally, the remedy to the warning is simply to add that package to your application (or loopbacks) dependency chain.

I will admit that this is the first time I have interacted with peerDependencies, and I was searching to the solution for this issue of needing to be siblings accessing the same libraries. I have no practical experience with whether this path is fraught with future heartache.

[ssh24]

@slnode test please

[ssh24]

@raymondfeng Any objection on using peerDependencies?

[ssh24]

@slnode test please

[raymondfeng]

@ssh24 I don't have strong objections to peerDependencies. But it probably only issues a warning if it's not satisfied.

[ssh24]

@raymondfeng I believe it throws an error when the peer dependencies are not satisfied. That is why I had to update a few downstream dependency connectors to use the latest 4.0.0 connector. More on the docs here.

[ssh24]

@slnode test please

[bajtos]

Please read also strongloop/loopback#275, loopback-connector was created specifically to allow each specific connector to come up with its own copy of that module.

[bobbysmith007]

This issue seems to be a recapitulation of strongloop/loopback#275.

That issue was because folks had their own private loopback-datasource-jugglers and thats a problem when sharing object models. This ticket is essentially that everyone has their own private loopback-connectors and are sharing object models which appear-to-be but are not the same types -- and thats the problem.

I don't know what the right or wrong answer here is (because managing complex dependencies is hard), but shared object models really aught to be shared or we cannot ever use the type information effectively (because myConnector instanceof Connector will never be true if we both have a different copy of connector).

To me (with much less experience in node than all of you) it seems that all loopback related modules should be loaded at once - in one dependency depth. That seems to be the point of peerDependencies - we are all sharing one shared object model. The top level loopback or loopback application should be responsible for listing all related modules that need to be loaded into one dependency space.

[bajtos]

@bobbysmith007 thank you for chiming in!

but shared object models really aught to be shared or we cannot ever use the type information effectively (because myConnector instanceof Connector will never be true if we both have a different copy of connector).

I our experience so far, there is no reliable way to ensure that after npm install finished, there will be only one instance of a given module (e.g. loopback-connector). The fact that there may be multiple copies of the same module is actually an important feature of Node.js/npm, as it solves the problem known as "dependency hell".

Yes, it means we cannot rely on instanceof operator to detect inheritance. If there is a code in LoopBack that relies on instanceof, then we need to fix it.

To me (with much less experience in node than all of you) it seems that all loopback related modules should be loaded at once - in one dependency depth.

Let me show you an example why this is not possible.

• a core package loopback-connector-mysql is depending on latest loopback-connector@^4.0.0, together with other 10+ connectors we are maintaining.
• a community-maintained package loopback-connector-foobar depends on loopback-connector@^2.0.0, together with many other community maintained connectors that haven't been updated to the latest loopback-connector yet.

As an application developer who wants to use both mysql and foobar connectors, if there was a requirement to have a single loopback-connector version in the app, you would have to search for loopback-connector-mysql version that is still using 2.x version of loopback-connector. What's even worse, loopback itself depends on loopback-connector (to provide the built-in memory connector), therefore the app developer would be forced to use an outdated loopback version too!

Now in most cases, the app developer won't be aware of this problem and they will happily happily install the latest versions of all dependencies (loopback, loopback-connector-mysql, loopback-connector-foobar). I believe no warning will be printed by npm in such case, and there will be at least two copies of loopback-connector in node_modules:

• node_modules/loopback-connector
• node_modules/loopback-connector-foobar/node_modules/loopback-connector

Obviously, the code relying on instanceof won't work as a result; and the user will have difficult times figuring out what went wrong.

Let's consider LoopBack maintainers too: to make a breaking change in loopback-connector, we would have to update all connectors (including the community-maintained ones) immediately after a new semver-major version of loopback-connector was released. That would cause breaking changes to be very expensive and even more difficult to get through.