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

Allow to setup an execution context. #147

Open
strayiker opened this issue Feb 9, 2019 · 11 comments
Open

Allow to setup an execution context. #147

strayiker opened this issue Feb 9, 2019 · 11 comments

Comments

@strayiker
Copy link

strayiker commented Feb 9, 2019

It would be nice to have an option to setup/teardown the context to be able to open the DB connection once and reuse it for all migration steps.
Perhaps, in the form of the --context option, which is similar to --store, accepts a module with:

exports.setup = async () => ({ db: await makeConnection() });
exports.teardown = async ctx => { await ctx.db.close(); };

Then pass the result of setup to each up/down function.

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 11, 2019

Hey @strayiker, I think you can do this without an api addition to the tool. Just setup your db as a singleton in a file imported into each migration. Is there a reason you can think of which makes adding api surface area to support this explicitly being better than implementing it within your own migrations?

For example, I use @wesleytodd/pg as a singleton wrapper and then in my migrations I just do this:

const pg = require('@wesleytodd/pg')

module.exports.up = function () {
  return pg.query('...')
}

Does that work for you?

@strayiker
Copy link
Author

strayiker commented Feb 11, 2019

@wesleytodd Yeah, i think it should work, I can't check it right now. But it's not a good solution, in my opinion. There is no place to destroy the connection and the idea of instantiating something inside a package seems wrong in general.


Besides, I can't instantiate the mongodb connection in this way because it asyncronous and can't be connected in global scope.

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 12, 2019

Hm, well it is just one of many patterns you can follow to achieve what you are looking for. You might also consider creating your own wrapper:

module.exports.up = setupConn(function (conn) {
  conn.query('...')
})

In this way you could manage the connection explicitly without a global singleton but also without adding api to this package.

Besides, I can't instantiate the mongodb connection in this way because it asyncronous and can't be connected in global scope.

The example above is also asynchronous, you can take a look at how that package does it if you need an example to work off of.

@strayiker
Copy link
Author

@wesleytodd This is the same as to setup connection inside an up/down functions. The main benefit of my suggestion is option to create connection once for all migrations and ability to destroy it gracefully in the end (what can't be done if we create connection in the global scope).

@wesleytodd
Copy link
Collaborator

Interesting, yes teardown is currently not possible without wrapping the js api (single connection is still just a singleton pattern away).

TBQH a few things could be made a lot better by just improving the js api and letting users who want specific features wrap this lib. But for this case I can see this might be a good addition to the library if we can find a good way to expose this functionality.

That being said, I am coming around to the idea because it would solve the teardown problem in a way which is nor currently done. As for the implementation, I would much rather have a more generic approach. Let me think a bit more on it...

@quex46
Copy link

quex46 commented Apr 22, 2019

Having context in each migration would be very convenient. It is native for sequelize/knex users, gives more flexibility and helps keep migrations cleaner.

@zebulonj
Copy link
Contributor

zebulonj commented Nov 15, 2019

I find myself looking for something similar, but for a different use case. I already use a wrapper of the JS API, to manage initialization and teardown or the database connections, but I have a need to move beyond the database singleton that I'm currently importing into each migration. What I'd like to be able to do is pass a context to Set::up(), as in...

migrate.load(CONFIG, (loadErr, set) => {
    if (loadErr) {
        reject(loadErr);
        return;
    }

    set.up(migration, context, upErr => {
        if (upErr) {
            reject(upErr);
            return;
        }

        resolve();
    });
});

...and have that context injected into each migration, as in...

module.exports.up = function(context, done) {
  // ...
}

For what it's worth... I recognize that this ☝️ approach would be a breaking change if it was introduced.

@wesleytodd
Copy link
Collaborator

I think that this feature will be in the next major version. The api for it will probably not be what you show here as it would be optional and not be passed via the arguments to up/down. But the idea of having a "context" which is accessible in the migrations will for sure be there.

@zebulonj
Copy link
Contributor

@wesleytodd You have any sense of when that next major version will drop? Anything I can do to help move it forward?

I'm looking in the code and was noodling on the idea of passing that context as the this to the up/down implementations...

module.export.up = function() {
  const { db } = this;

  return db.query(...);
};

I'm not vested in any particular interface, but that might allow if to be a non-breaking change.

@wesleytodd
Copy link
Collaborator

wesleytodd commented Nov 15, 2019

@zebulonj, there might be ways we can start working together. I was working on it pretty regularly for a bit, but I have had some changes at work and in my OSS commitments which have taken precedence. The first thing you might take a look at is the progress here: https://github.com/migratejs

Right after I post this Later this weekend I will also go and create a discussions repo over there to have a place to discuss the design and progress across the repos.

@davestimpert
Copy link

As a workaround, you can add a filter function like this:

filterFunction: (file) => {
require(${migrationsDirectory}/${file}).addContext({ db })
return file
}

And then require that each of your migrations has an addContext function which sets state in the module.

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

No branches or pull requests

5 participants