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

Changed the mongodb dependency from an installed dependency to a peer de... #2

Closed
wants to merge 1 commit into from

Conversation

mrsimonemms
Copy link

...pendency as there are breaking changes between mongodb v1.x and v2.x

… dependency as there are breaking changes between mongodb v1.x and v2.x
@okv
Copy link
Owner

okv commented Mar 26, 2015

Hi.
Adapter directly requires mongodb package to use MongoClient for establish connection with db.
So that's why it's direct dependency (currently mongodb >=1.3.0), e.g. if your app use version of mongodb lower then 1.3.0 then it will be two mongodb versions installed - for your application and for east adapter.
If current adapter is not compatible with mongodb 2.x (i don't know, i'm still using 1.x) i guess we should set that requirement at dependencies rule (something like mongodb >=1.3.0 < 2.x) for quick fix. Or alternatively (better way) we should learn adapter to work with mongodb 2.x (along with 1.x of course), then dependencies mongodb >=1.3.0 will be ok. You can do it if you want =)
Is it right for you? What is your particular case?

@mrsimonemms
Copy link
Author

My use case is we have a PHP app, but use this package to manage our MongoDB migrations. We're creating JSON schema definitions of our API with references. The original version (what I wrote) had the references using "@ref", but they've been unilaterally changed to using "$ref" (as per the JSON schema. "$ref" fails when importing using v2 of the mongodb driver. Because my colleague has decided to change everything without consulting anyone, we need to use a 1.x version of the mongodb driver.

The reason I chose to use the peerDependency rather than changing the dependency is that it gives you more flexibility as a package and keeps nicer within the Strategy Pattern - this package doesn't use the mongodb driver per se, but it requires it for use. This, to my understanding, is what the peerDependency is for. Also, my colleague shouldn't be dictacting how your package works which is why I decided against the ">= 1.3.0 <2.0.0" versioning.

Also, we're not using this package as a global (we're using the binary in /node_modules/.bin/. There is a discussion to be had (as owner of this package, you should have final say) on the impact on people using this as a global module - they'll need to have mongodb installed globally.

My tuppence ha'penny, for the record, is that the peerDependency gives us greater flexibility. The developer implenting this in their project can easily install a specific version of mongodb either as a global or local module. Also, if we used the >= 1.3.0 < 2.0.0 solution then, when mongodb gets to v3 and beyond, this package will be considered out of date very quickly - this be wrong, seeing how it's a thin-wrapper that exposes migration/rollback and lets the developer use the mongodb instance however they choose.

@okv
Copy link
Owner

okv commented Mar 26, 2015

I see your point.
Dependency mongodb >=1.3.0 < 2.x is not an option if adapter works with mongodb 2.x.
Should mongodb be dependency or peerDependency - is a question, i can't say unambiguously right now. And as for me it's not so big question because you can manage mongodb vesrion by setting it directly at package.json (see example below).
Have you tried to set monogdb version along with east postgres version at your package.json, e.g. if you set:

"mongodb": "1.3.6",
"east-mongo": "0.1.2"

and do npm install you will get east postgres which is uses mongodb 1.3.6 (because npm reduces duplication of dependencies tree)

@mrsimonemms
Copy link
Author

Well bugger me. I've learnt something there. I've been a NodeJS developer for 3 years and have never had to specify a dependency like that before. Specifying the mongodb package in the package.json file has worked.

Guess that negates the whole purpose of this pull request then...

I'll be writing a MySQL adapter for east soon so will speak again when that's done.

@okv
Copy link
Owner

okv commented Mar 26, 2015

Glad to hear about mysql adapter we already had for sqlite and postgres (ive just added link to it on east page).

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.

2 participants