building5 / sails-db-migrate

db-migrate integration for sails.js
MIT License
61 stars 32 forks source link

removed dependancy on sails to work with stand alone waterline #16

Closed mbrooks closed 8 years ago

mbrooks commented 9 years ago

First stab at making a sails-db-migrate compatible with both sails and waterline without sails setups. I can already tell you the configuration import may need more work to work properly with the way sails does things, but let me know what you think!

leedm777 commented 9 years ago

I don't get why a project named sails-db-migrate would want to run without sails. Even if you could get it to work, you would be basically duplicating the entire Sails configuration process. Which means if they change their configuration process (like adding support for .yml config files), all this stuff will break.

Plus, in order to use this anyone who is using Waterline without Sails will have to have a very Sails-like configuration. The intersection of people a) using Waterline b) not using Sails and c) happens to use a Sails config structure is probably very small.

Rather than go down this path, I see a couple of better options.

  1. Your framework doesn't provide a configuration layer, so you provide that yourself. In that case, I would just put the database configuration into a database.json file. This is what db-migrate uses natively for its configuration, and you can just pull it in for configuring Waterline when starting up your app.
  2. You are using a different framework, which has its own configuration process. Write an other-framework-db-migrate which does something similar to sails-db-migrate to run the other framework's configuration layer, and passes that config off to db-migrate.
mbrooks commented 9 years ago

Well, it was worth a shot, but I understand your point of view. It was just quicker to use this code rather than write something from scratch that I have to maintain, etc.

Possible 3rd option: How about I try to detect if the sails module can be loaded (if this is even possible with the way npm works) and pull config from sails if possible? It could fall back to the config import if sails is unavailable. Or, is there no hope in getting this pushed into project?

leedm777 commented 9 years ago

Possible 3rd option:

That's still a lot of config parsing junk in here that doesn't really belong. If you could find a way of invoking the Sails config parser independently of lifting Sails, I'd be happy with a patch that used that for getting the database config.

I don't know what other constraints you have, but my guess is that you would be happier if you just put your database config in database.json and ran db-migrate directly. And maybe something like grunt-db-migrate if you want to invoke it from grunt.