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.

Sign up for GitHub

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

Jump to bottom

fix node_modules problem #1

Closed
wants to merge 1 commit into from
Closed

fix node_modules problem #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 1 addition & 7 deletions lib/load-adapter-from-app-dependencies.js
View file
Open in desktop
Expand Up @@ -39,18 +39,12 @@ module.exports = function loadAdapterFromAppDependencies(adapterPackageName, dat
// └─ └ ┴└─└─┘┴ ┴ ┴ ┴┴ ┴ ┘└┘└─┘─┴┘└─┘────┴ ┴└─┘─┴┘└─┘┴─┘└─┘└─┘ └ └─┘┴─┘─┴┘└─┘┴└─ ─┘

// Since it is unknown so far, try and load the adapter from `node_modules`
sails.log.verbose('Loading adapter (`%s`) from this app\'s `node_modules/` directory...', adapterPackageName);

// Before trying to actually require the adapter, determine the path to the module
// relative to the app we're loading:
var userlandDependenciesPath = Path.resolve(sails.config.appPath, 'node_modules');
var adapterPackagePath = Path.join(userlandDependenciesPath, adapterPackageName);


// Now try to require the adapter from userland dependencies (node_modules of the sails app).
var adapter;
try {
adapter = require(adapterPackagePath);
adapter = require(adapterPackageName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

} catch (e) {
// If there was a problem loading the adapter,
// then check to make sure the package exists in the `node_modules/` directory.
Expand Down