Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect dependency on juggler and strong-remoting #5

Closed
bajtos opened this issue Oct 6, 2014 · 9 comments
Closed

Incorrect dependency on juggler and strong-remoting #5

bajtos opened this issue Oct 6, 2014 · 9 comments

Comments

@bajtos
Copy link
Member

bajtos commented Oct 6, 2014

The connector directly depends on loopback-datasource-juggler, which is a recipe for problems, as it causes multiple instances of the juggler to be loaded in the Node process.

Since loopback depends on the remote connector itself, this problem happens in every loopback application.

$ find . -name 'loopback-datasource-juggler'
./node_modules/loopback/node_modules/loopback-connector-remote/node_modules/loopback-datasource-juggler
./node_modules/loopback-datasource-juggler

All other connectors depend on loopback-connector - see strongloop/loopback#275.

/to @kraman @ritch

@bajtos bajtos added the bug label Oct 6, 2014
@bajtos
Copy link
Member Author

bajtos commented Oct 6, 2014

I checked the code, apparently this connector uses juggler's internals that are not part of loopback-connector, the solution thus won't be trivial.

I am proposing two steps:

  • make juggler a peer dependency as a quick fix removing multiple juggler instances
  • refactor the code loopback-datasource-juggler, loopback-connector and loopback-connector-remote so that the dependency on juggler is no longer needed.

@bajtos bajtos self-assigned this Nov 5, 2014
@bajtos
Copy link
Member Author

bajtos commented Nov 5, 2014

make juggler a peer dependency as a quick fix removing multiple juggler instances

This won't solve the problem, it will only push the copy of loopback-datasource-juggler from the connector to loopback itself. We would have to change loopback to peer-depend on the remote connector to fix the issue via peer dependencies.

In the light of the above, I prefer to do a proper fix and refactor the code to remove the dependency on juggler.

@ritch
Copy link
Member

ritch commented Feb 4, 2015

What if we just move the juggler to dev deps and always use the version provided by the app's loopback node_modules?

@bajtos bajtos added #tob and removed #sprint59 labels Feb 4, 2015
@chandadharap chandadharap removed the #tob label Feb 5, 2015
@bajtos
Copy link
Member Author

bajtos commented Feb 5, 2015

What if we just move the juggler to dev deps and always use the version provided by the app's loopback node_modules?

That is an option too. It will require some change in loopback and/or juggler to expose the stuff used by remote connector in a public API that is accessible by the connector.

@ritch
Copy link
Member

ritch commented Feb 5, 2015

It will require some change in loopback and/or juggler to expose the stuff used by remote connector in a public API that is accessible by the connector.

Why? Shouldn't the connector be able to get at the juggler via require('loopback-datasource-juggler')?

@bajtos
Copy link
Member Author

bajtos commented Feb 6, 2015

No necessarily. Once we change juggler from peer dep to a regular dep, it is possible to end up with the following layout:

node_modules
  - loopback-connector-remote
  - loopback
    - node_modules
      - loopback-datasource-juggler

Depending on juggler being require-able from remote-connector is IMO brittle and sort of a bad practice.

@bajtos bajtos changed the title Incorrect dependency on juggler Incorrect dependency on juggler and strong-remoting Aug 31, 2016
@bajtos
Copy link
Member Author

bajtos commented Aug 31, 2016

We are also depending on strong-remoting, which causes similar issues. Especially now that strong-remoting@3 has major breaking change compared to strong-remoting@2 (see strongloop/strong-remoting#343).

As a temporary stop gap, I am thinking about cutting a new major version of loopback-connector-remote that will support only LoopBack v3+, and keep the current version line 1.x for loopback v1 and v2.

@bajtos
Copy link
Member Author

bajtos commented Sep 1, 2016

As a temporary stop gap, I am thinking about cutting a new major version of loopback-connector-remote that will support only LoopBack v3+, and keep the current version line 1.x for loopback v1 and v2.

done.

@dhmlau
Copy link
Member

dhmlau commented Aug 23, 2018

Since the last comment was almost 2 years ago, closing due to inactivity. Please create a new issue if you are still running into problems.

@dhmlau dhmlau closed this as completed Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants