Proposing options.origin as a Promise.
#44
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 54 54
Branches 19 19
=====================================
Hits 54 54
Continue to review full report at Codecov.
|
| @@ -48,7 +48,7 @@ app.use(cors()); | |||
| * CORS middleware | |||
| * | |||
| * @param {Object} [options] | |||
| * - {String|Function(ctx)} origin `Access-Control-Allow-Origin`, default is request Origin header | |||
| * - {String|Function(ctx)|Promise} origin `Access-Control-Allow-Origin`, default is request Origin header | |||
There was a problem hiding this comment.
{String|Promise<string>} may be better
There was a problem hiding this comment.
But that would be a breaking change wouldn't it? It supports functions as is.
There was a problem hiding this comment.
promise is still function, right? aren't you upgrade it from synchronous normal function to promise?
| if (typeof options.origin === 'function') { | ||
| // FIXME: origin can be promise | ||
| origin = options.origin(ctx); | ||
| return resolve(options.origin || requestOrigin); |
There was a problem hiding this comment.
seems it could be return resolve(requestOrigin)
There was a problem hiding this comment.
@dead-horse shall we regard it as a break change and rewrite the ancient test? or do we still need to support normal function?
There was a problem hiding this comment.
we should keep support normal function as well.
|
@rapzo lgtm. could you rebase your comment :) |
|
I think we can rewrite this module by async function so it will be much clear. :) |
|
sounds good. so |
|
@dead-horse yeah, i got that covered in #43 |
|
Ping |
|
Gonna bury this PRs. This repo is dead. |
|
Really sad. @dead-horse can you maybe even provide a statement whether this project is really dead or you seek a new maintainer? |
As it was pointed at L59