CLI: adds support for asynchronous knexfile loading #3748
Conversation
bin/cli.js
Outdated
| if (typeof config === 'function') { | ||
| config = config(); | ||
| } | ||
| if (config instanceof Promise) { |
There was a problem hiding this comment.
This is redundant, you can simply await config() and if it's not a promise, it will just return the value.
| @@ -65,6 +65,33 @@ module.exports = { | |||
| ); | |||
| }); | |||
|
|
|||
| it('Run migrations with knexfile returning function passed', () => { | |||
There was a problem hiding this comment.
Can you also add same tests for seeder execution? Ideally the two should work symmetrically.
|
One thing that I'm concerned about is that this implements support for async knexfile usage in CLI context, but not for general knex usage, meaning knexfile is no longer universally applicable. You may want to check out #3497 - it may potentially address your use-case; if it's not, we can proceed with your implementation. |
|
@kibertoad you're right, #3497 addresses my needs for now. |
|
Your extension is pretty non-intrusive; if the rest can be done in similarly non-intrusive way, and you could provide documentation for that, I think it could be merged. |
@kibertoad I am not sure I follow. I can't see in the codebase where it loads |
|
@flovilmart lib/knex.js -> function Knex(config) { Now I'm thinking whether or not user would expect promise to be supported here; it actually probably is going to be way simpler if they would resolve knexfile from the promise up-front and then passed that to the knex. So you are right, no additional changes are needed. |
|
awesome. Then again, I am not particularly attached to this PR, but it make is easier to deal with async knexfiles, the current implementation supports only for the connection, which is great and just what I need. |
|
@flovilmart Could you also submit PR to https://github.com/knex/documentation to mention that knexfile can return a promise? |
|
@kibertoad opened this one: knex/documentation#259 |
|
There's a minor conflict, could you please fix it? |
|
Thanks! |
|
Thanks for the quick response @kibertoad. I praised your commitment in my team this morning 🙌 |
|
@flovilmart Ahaha, I'm touched! How urgently do you need this feature? I can cherry-pick this change to 0.20 prior to next major release, since it looks non-breaking. |
|
it's not urgent at all, we're in the process of integrating migrations to our codebases. If you wanna go ahead and cherry pick, that's appreciated nonetheless :) |
(cherry picked from commit 41d02ba)
(cherry picked from commit 41d02ba)
|
Released in 0.20.14. |
|
Thanks! |
We plan to use knex CLI but our credentials are safely stored in a vault, which requires to be accessed asynchronously.
This PR adds the support for letting users have the knexfile options returned asynchronously.