Add ability to retrieve context asynchronously before the migrations run #453
Conversation
|
Hi, |
|
Hi, is there any chance that this PR would merge in near future? Thanks. |
src/umzug.ts
Outdated
| @@ -483,6 +480,23 @@ export class Umzug<Ctx extends object = object> extends emittery<UmzugEvents<Ctx | |||
| }); | |||
| } | |||
|
|
|||
| private async getContext(): Promise<Ctx> { | |||
| const isPromise = (_ctx: object | Function) => { | |||
There was a problem hiding this comment.
Do we need this? Can't we just check if it's a function and await context() if it is?
There was a problem hiding this comment.
Unfortunately, Promise isn't a function and this comparison is required. We could use different mechanics to detect promise, such either typeof _ctx.constructor === 'function'; or Boolean(typeof value.then === 'function');, but in my opinion, current implementation Is better way to do that. WDYT?
There was a problem hiding this comment.
I mostly mean that you can await a non-promise too. So we can keep lines 231-234 basically the same but put an await before each ternary statement option, then I don't think we'd even need this function.
There was a problem hiding this comment.
The context retrieving action is a significant one, and grouping it into the method getContext increases code readability. As for rest, if we would add await before each ternary, the Promise branch won't execute at all.
There was a problem hiding this comment.
That's true. I'm fine with keepinh getContext, encapsulating the logic in a function, as long as we get rid of this toString based promise checking. It's brittle (likely won't work with non-native promises like bluebird, etc.) and it doesn't seem necessary.
There was a problem hiding this comment.
I changed the Promise detection implementation. Please review.
There was a problem hiding this comment.
Any updates on it? :)
There was a problem hiding this comment.
I left a suggestion to explain what I mean. Please add a failing test if you think my suggestion doesn't cover everything.
There was a problem hiding this comment.
I simplified the method according to your suggestion. Thanks!
src/umzug.ts
Outdated
| const isPromise = (_ctx: object | Function | PromiseLike<unknown>) => { | ||
| return 'then' in _ctx && typeof _ctx.then === 'function'; | ||
| }; | ||
|
|
||
| let { context = {} as Ctx }: UmzugOptions<Ctx> = this.options; | ||
|
|
||
| if (isPromise(context)) { | ||
| context = await Promise.resolve(context as Promise<Ctx>); | ||
| } else if (typeof context === 'function') { | ||
| context = await (context as () => Promise<Ctx> | Ctx)(); | ||
| } | ||
|
|
||
| return context; |
There was a problem hiding this comment.
Sorry, I clearly haven't been explaining this very well. We do not need to check if the context is a promise. The fact that this function is async will ensure it's one anyway, and a the js engine will flatten promises for us. The implementation of this method can and should be much simpler and have less risk of gotchas.
| const isPromise = (_ctx: object | Function | PromiseLike<unknown>) => { | |
| return 'then' in _ctx && typeof _ctx.then === 'function'; | |
| }; | |
| let { context = {} as Ctx }: UmzugOptions<Ctx> = this.options; | |
| if (isPromise(context)) { | |
| context = await Promise.resolve(context as Promise<Ctx>); | |
| } else if (typeof context === 'function') { | |
| context = await (context as () => Promise<Ctx> | Ctx)(); | |
| } | |
| return context; | |
| const { context = {} } = this.options; | |
| return typeof context === 'function' ? context() : context; |
The tests you added are good - they should pass if you make this change. Assuming they do, I'm happy to merge.
test/umzug.test.ts
Outdated
| return { | ||
| // Eg: externalData, | ||
| innerValue: 'text', | ||
| }; |
There was a problem hiding this comment.
nit: this comment is confusing/not necessary - the one above explaining the use case you're trying to replicate is, but no need for the extra comment here:
| return { | |
| // Eg: externalData, | |
| innerValue: 'text', | |
| }; | |
| return { innerValue: 'text' }; |
|
Released as 3.0.0-beta.16 |
It often needs that we should do some preparation prior to context could be provided to migrations modules. It could be the initialization of database backend storages, etc. The old behavior keeps untouched, but this PR adds the advantage to use the asynchronous functions to retrieve context.