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

Make loopback-datasource-juggler a peer dep #85

Merged
merged 1 commit into from
Dec 6, 2013

Conversation

raymondfeng
Copy link
Member

/to @ritch

@ritch
Copy link
Member

ritch commented Dec 6, 2013

lgtm

raymondfeng added a commit that referenced this pull request Dec 6, 2013
Make loopback-datasource-juggler a peer dep
@raymondfeng raymondfeng merged commit 383f420 into master Dec 6, 2013
@raymondfeng raymondfeng deleted the feature/juggler-dep branch December 6, 2013 18:31
@slnode
Copy link

slnode commented Dec 6, 2013

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/465/

@bajtos
Copy link
Member

bajtos commented Dec 12, 2013

@raymondfeng @ritch do you remember why was this change made?

Quoting from strongloop/loopback-workspace#28:

peerDependencies are meant to control versions of peer dependencies more than control that they are actually installed.

In other words, we are sort of saying "you can depend on loopback only if you depend directly on loopback-datasource-juggler too".

@rmg
Copy link
Member

rmg commented Dec 12, 2013

Installation does seem to be part of the deal according to the peerDependecies announcement blog post, but there doesn't seem to be any docs for it.

Another thing to consider is that loopback does require() loopback-datasource-juggler directly, which makes me question if it this is close enough to the plugin style of relationship that peerDependencies is meant to address.

This also breaks npm-link, if I recall correctly.

@raymondfeng
Copy link
Member Author

Here is the user experience leads to the peerDependencies:

  1. You create a loopback project using 'slc lb project' command. The package.json declares 'loopback' as the dependency.
  2. Run 'npm install loopback-connector-mongodb --save' to add mongo support. (Connectors are plugins).

Before peerDependency is adopted, we end up with the following tree:

myapp --> loopback --> loopback-datasource-juggler
--> loopback-connector-mongodb --> loopback-datasource-juggler

With peerDependency, now we have:

myapp --> loopback
myapp --> loopback-datasource-juggler ('npm install' adds this peer at the same level as loopback)
myapp --> loopback-connector-mongodb

@rmg
Copy link
Member

rmg commented Dec 12, 2013

Ah, and it was added as a devDependency to get around the fact that loopback-datasource-juggler won't be installed within loopback if you run npm install from a checkout.

In that case it makes sense to use the same technique in loopback-workspace.

@raymondfeng
Copy link
Member Author

Yes, exactly.

@innerdaze
Copy link

I don't know if this is new information for you, but running npm install loopback on the latest version fails unless you have explicitly npm installed loopback-datasource-juggler.

Adding loopback-datasource-juggler as a devDependency to loopback currently isn't forcing npm to download loopback-datasource-juggler so the install fails.

Are we convinced the peerDependencies are a stable npm feature?

@raymondfeng
Copy link
Member Author

Thank you for bringing it to our attention.

  1. The loopback module is typically used as a dependency to your application. In that case, npm install should work.
  2. npm install -g will install the peer dependencies.

If you just run ‘npm install loopback’ in a directory that doesn’t have package.json with loopback as a dependency, it will fail.

Raymond

On Feb 14, 2014, at 1:48 AM, lsdriscoll [email protected] wrote:

I don't know if this is new information for you, but running npm install loopback on the latest version fails unless you have explicitly npm installed loopback-datasource-juggler.

Adding loopback-datasource-juggler as a devDependency to loopback currently isn't forcing npm to download loopback-datasource-juggler so the install fails.

Are we convinced the peerDependencies are a stable npm feature?


Reply to this email directly or view it on GitHub.

@rmg
Copy link
Member

rmg commented Feb 14, 2014

Interesting.

I just did npm install loopback in a clean directory with a fresh package.json (npm init, all defaults) and it installed both loopback and loopback-datasource-juggler without complaint. Running npm ls warns that [email protected] is extraneous.

If I do the same thing, but do npm install --save loopback, both loopback and loopback-datasource-juggler are added to dependencies.

I cleared the cache with npm cache clean both times and I'm using [email protected] and [email protected].

@ritch
Copy link
Member

ritch commented Feb 14, 2014

@lsdriscoll - what version of npm / node are you using?

Hint:

npm -v
node -v

@innerdaze
Copy link

@ritch - Apologies for the late reply, left my phone at work all weekend -_-
npm: 1.3.14
node: 0.10.22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants