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

Loopback expects loopback-datasource-juggler, but no longer requires it #28

Merged
merged 1 commit into from
Dec 13, 2013

Conversation

rmg
Copy link
Member

@rmg rmg commented Dec 12, 2013

Not sure if this change should be in loopback-workspace or in loopback.

Notice that CI is still failing, but now it is due to a failing test, not a test crashing because a require() failed.

/to @Schoonology @ritch @raymondfeng @bajtos

@slnode
Copy link

slnode commented Dec 12, 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-workspace/130/

@Schoonology
Copy link
Contributor

Considering what the Workspace does now, this Looks Good to Me. :shipit:

@bajtos
Copy link
Member

bajtos commented Dec 12, 2013

hang on.

@bajtos
Copy link
Member

bajtos commented Dec 12, 2013

loopback-datasource-juggler is a peerDependency of loopback, which is already defined as a dependency of loopback-workspace in it's package.json.

When you run npm install for this project, loopback-datasource-juggler is correctly installed.

Seems there might be a bug in our CI tool?

@bajtos
Copy link
Member

bajtos commented Dec 12, 2013

If we don't want/cannot fix the CI tool now, then a better fix may be to require the loobpack-datasource-juggler as devDependency instead of a regular dependency?

@rmg
Copy link
Member Author

rmg commented Dec 12, 2013

Ah, I think I see the problem now. Because loopback is "installed" from Artifactory using strong-install instead of npm, it doesn't take loopback's peerDependencies into account when generating loopback-workspace's dependencies.

That said, from my understanding of peerDependencies, it is meant to control versions of peer dependencies more than control that they are actually installed.

This dependency chain and use of peerDependencies in loopback seems wrong to me some how...

@slnode
Copy link

slnode commented Dec 12, 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-workspace/131/

@bajtos
Copy link
Member

bajtos commented Dec 12, 2013

That said, from my understanding of peerDependencies, it is meant to control versions of peer dependencies more than control that they are actually installed.
This dependency chain and use of peerDependencies in loopback seems wrong to me some how

Hmm, maybe you are right. Let's discuss that in strongloop/loopback#85. In the meantime, pick whatever solution works best for you.

Because loopback lists loopback-datasource-juggler as a peerDependency but
still require()s it directly, it must be present for tests to run.

Under normal usage, the loopback dependency would pull in
loopback-datasource-juggler. This is mainly for dev/CI.
@slnode
Copy link

slnode commented Dec 12, 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-workspace/133/

@rmg
Copy link
Member Author

rmg commented Dec 12, 2013

Per the discussion in strongloop/loopback#85, loopback-datasource-juggler should be a devDependency.

@bajtos @raymondfeng and/or @Schoonology confirm good to merge now?

rmg added a commit that referenced this pull request Dec 13, 2013
Loopback expects loopback-datasource-juggler, but no longer requires it
@rmg rmg merged commit 2b65492 into master Dec 13, 2013
@rmg rmg deleted the feature/fix-missing-dependency branch December 13, 2013 03:49
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.

5 participants