The generators and the server fail if the mongodb-wrapper isn't installed locally. #209

Closed
MiguelMadero opened this Issue Oct 3, 2012 · 13 comments

Comments

Projects
None yet
5 participants
Contributor

MiguelMadero commented Oct 3, 2012

We should use the mongodb-wrapper installed globally.

Contributor

larzconwell commented Oct 3, 2012

People usually like to install modules locally so that they're easier to manage and update. If they're installed globally people tend to let them get out of date. Also locally installed modules makes it easier to manage dependency hell between multiple projects.

Contributor

polotek commented Oct 3, 2012

We should do something to make sure people know that they will need certain modules installed when they use certain features of geddy. What's the story for this right now? The first thought that came to me was that using the mongo adapter should result in mongodb-wrapper being put in your package.json for you. Or at least prompting you to do so and then running npm install. At the very least geddy should print out a sensible warning if you start it up and it doesn't have all the necessary deps.

Contributor

larzconwell commented Oct 3, 2012

Currently when you start the server up and you don't have the needed database dependencies installed(monodb_wrapper, pg) it'll throw with a message saying install whatever module.

Here's the message:
"Module "<node_module_here>" could not be found as a local module. Please make sure there is a node_modules directory in the current directory, and install it by doing "npm install <node_module_here>""

Contributor

larzconwell commented Oct 3, 2012

Also when you have the metrics options set, it'll throw on server boot with the same message if it's not installed. Same with socket.io as well.

Contributor

MiguelMadero commented Oct 3, 2012

The message is fine and useful. I open this issue because I thought it might be a good idea to fault-back to the global module in case there's nothing locally. I agree, it's easier to manage dependencies if we have them locally, I guess the question is, should we force this or should we let each dev decide by supporting global modules?

Contributor

polotek commented Oct 4, 2012

We shouldn't be doing anything to support global modules. npm has a workflow that recommends local modules. Global modules are for things like cli commands. If a dev really wants to share the same module across projects, they have options. They can use symlinks or things like npm link from global.

Contributor

mde commented Oct 4, 2012

Our local-require command used to fall back to global, but I ripped that code out for the reasons @polotek laid out. However, we do need a better story for the generators when setting up a new model. Since we support per-model DB-engine, we can't make the assumption that there's a default DB config for all models in the app. What should the ideal workflow be in the case where you're using a SQL store that requires you create an actual table?

Contributor

polotek commented Oct 4, 2012

I'm not familiar enough with the generators yet. But I do think there should a default datastore. And then you explicitly override it for certain models. Maybe that's not what you're getting at though. Is there a flag to the model generator to tell it what datastore to expect?

Contributor

MiguelMadero commented Oct 4, 2012

Considering what @polotek mentioned I think that the current behavior is correct. I thought using a global module would make sense. Since that's not the case, I think the current message is a perfect and clear solution:

"Module "<node_module_here>" could not be found as a local module. Please make sure there is a node_modules directory in the current directory, and install it by doing "npm install <node_module_here>""

About creating the table. I think that based on the current adapter we should be able to add this step. There might be an intention to do this already

[Added] app/models/todo.js
Creating table for Todo

Also, in the last pull request I sent with the changes to the tutorial, I made the memory adapter the default at the app level and removed this line from the models. This means that choosing mongo is now an explicit action. So an extra step of installing the needed package doesn't seem like much overhead.
Now I'm not sure this is the way you guys want to go, but it made sense to me so I changed it as I explained in the commit messages.

Contributor

MiguelMadero commented Oct 4, 2012

@polotek there're two places to set this. At the environment level (in config/dev|prod.js) or at the model level. In the current version scaffold/resource/model set it at the model level and it defaults to mongo. In the last PR I sent it's set at the environment level, it defaults to memory and we don't set anything at the model level, but you could still override it

Contributor

Techwraith commented Oct 4, 2012

@MiguelMadero I thought we changed it use the memory adapter already, if not, thanks for making that change!

Contributor

MiguelMadero commented Oct 4, 2012

I checked the diff and memory was the default at the model level, but I made it the default at the app level.

Contributor

MiguelMadero commented Oct 4, 2012

I'm closing this since modules should be local.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment