fix node_modules problem

[stanislavromanov]

Fixes part of balderdashy/sails#3732

[mikermcneil]

My understanding is that this would prevent users from running npm install sails-mysql to install the mysql adapter in their apps-- but maybe I'm missing something?

[stanislavromanov]

No, you can install whatever you wish and it will load just fine. What's the difference between: require('package') and require('./some/folder/node_modules/package').

If package is installed on project root e.g. ./, ./some/folder/app.js will go up and will find the package so we're good.

[mikermcneil]

@stanislavromanov If I'm understanding you correctly, there's still a problem (at least with the versions of Node and NPM I've used in the past). If code in sails-hook-orm (a dependency of sails, which is a dependency of your project) attempts to require('sails-mysql'), then Node will look at the dependencies of sails-hook-orm -- in its node_modules folder. And like you're saying, it will continue to look at higher and higher level directories that have a package.json, attempting to find the dependency. But for some reason or another, this just doesn't work, at least not on all versions of Node/NPM that we support (I started off trying to do it that way, and only changed it after it didn't work for some developers. I had to do the same thing for generators, view engines, hooks, socket.io adapters, and connect session adapters.)

All that said, it may be that this is fixed now on all versions of NPM/Node in common use today (i.e. Node >=0.10 and NPM >= 2.0). If that's the case, then we just need some more test cases that verify that this works (Travis can take care of testing on multiple versions of Node-- I'll make sure I push up a travis.yml file for this repo). I'd love to be able to simplify this code throughout the framework!

[stanislavromanov]

Okey, I get it that it will not work on old versions, however: Wouldn't it be wise to just drop support for 0.* versions of node and start supporting LTS 4.4.7?

[mikermcneil]

Wouldn't it be wise to just drop support for 0.* versions of node

@stanislavromanov as much as I'd love to be able to do that, there are a lot of folks still using Node 0.10.

I think the right thing to do here might be just to support and document a different default approach. I.e. instead of providing the string package name, providing the actual already-required reference. We'd still need to support the existing behavior, but anywhere throughout Sails we support providing a package name, we could change it to allow providing the already-required package itself. (The one exception will still be generators, where that isn't really an option-- but it should work everywhere else, and we already support that approach in some places)

[stanislavromanov]

Well it's fine by me because I already cloned it and using my fork.

The question I want to ask tho: If people do not bother to update their Node from 0.1 to stable LTS 4.x why are you so sure they would update their Sails.js? :)

[mikermcneil]

@stanislavromanov As I was experimenting with this, I was just reminded of another time where this matters: when you're developing using npm link

[mikermcneil]

@stanislavromanov One last important consideration is which installed module this actually grabs. The only sure-fire way to use require this way and still know what version you're getting is to include that version as a dependency (and obviously sails-hook-orm can't include every Sails adapter on NPM as a dependency.)

I think the right solution here is for sails-hook-orm to expect us to pass in the already-required adapter in our config (and also to maintain backwards compatibility for the current approach).

[cubico]

Hi guys,
I have a problem due to the dependence of modules. I want to execute the deferred .meta() - Model.find().meta({metadata1: value1}) - available in waterline 0.12.2 but the sails-hook-orm uses waterline 0.11.0 where it is not available within. Any suggestions? Thanks!
(Some hints: Node 0.12.7, Npm 3.8.7, Sails 0.12.3 | 0.12.4-rc3, MongoDb 3.3.1)

[mikermcneil]

@cubico Gotcha. OK, so thankfully this is possible now (whereas it wasn't a few months ago), and it's one of the main reasons I pulled out sails-hook-orm (with the help of the guys at Postman).

Here's how to do it:

1. fork sails-hook-orm and clone your fork locally
2. run npm install waterline@0.12.2 --save --save-exact to bump your local copy of sails-hook-orm to the desired version of waterline.
3. Change the package name in package.json (e.g. to @cubico/sails-hook-orm)
4. Then commit that and run npm version patch.
5. Then push everything up to your fork and run npm publish to publish your forked version on NPM
6. Finally, in your Sails app, run npm install @cubico/sails-hook-orm --save to add it as a dependency (you could also skip the NPM step and just add a dependency directly from GitHub-- it's just easier/cleaner to use NPM imo).

Then, if you lift your app, Sails should attempt to load your fork of the orm hook instead of the built-in version (because sails-hook-orm has sails.hookName==='orm' in its package.json).

Hope that helps!

[cubico]

+1. Thanks @mikermcneil ! I found a easy way in order to use the correct version of waterline without the waterline dependencies in sails-hook-orm module ( but I must remember not run npm again :D)

1. I install wateline at top of node_modules run npm install waterline@0.12.2 --save --save-exact as you comment.
2. and delete/rename node_modules/sails-hook-orm/node_modules/waterline and node_modules/sails-hook-orm/node_modules/waterline-criteria

... and seems it works for me. May be this is a PR and the solution is delete the dependency of waterline from package.json of this module. It is possible? Some "official" tests about this?

[mikermcneil]

@cubico glad you got it sorted! The issue we're stuck with is still npm 2 vs. npm 3. Since waterline is being required from sails-hook-orm, the semver range indicated in sails-hook-orm's package.json will be used to find it. And while we might be able to use some fanciness to make this work (manually resolving dir paths), it always ends up being npm-version dependent (since where your dependencies are installed varies). So far, the cleanest way to allow for this without causing cross-platform issues has been to allow installing different hooks.

btw: I think maybe what would be a good solution here is to publish a version of this hook with a tag specifically for your use case. i.e. so that way you could put "sails-hook-orm": "waterline12" in your package.json file. Happy to do that if it'd be helpful for you-- just, if you wouldn't mind, please create a PR to this repo with the waterline semver range in the package.json set to exactly what you need and at-mention me.

[mikermcneil]

@cubico @stanislavromanov btw I recently was working on this in sails-generate, and here's what I did there: https://github.com/balderdashy/sails-generate/blob/ee251bdceaa2d918e2db40a5476723f5e4100032/lib/load-generator-def.js

Also, for some more background on the NPM version discrepancies, see https://github.com/balderdashy/sails-generate/blob/ee251bdceaa2d918e2db40a5476723f5e4100032/lib/core-generators/new/after.js#L80-L255