added support for dynamical upstream configuration #178
Conversation
replyOptions.getUpstream must be specified for this
index.js
Outdated
| @@ -117,7 +117,7 @@ function generateRewritePrefix (prefix, opts) { | |||
| return '' | |||
| } | |||
|
|
|||
| let rewritePrefix = opts.rewritePrefix || new URL(opts.upstream).pathname | |||
| let rewritePrefix = opts.rewritePrefix || new URL(opts.upstream || 'http://empty').pathname | |||
There was a problem hiding this comment.
Isn't this http://empty default value avoidable?
There was a problem hiding this comment.
I think so, we should just use a default pathname if no upstream is there.
There was a problem hiding this comment.
We need to provide any valid upstream to URL constructor, in another case, we'll have to handle the exception that the URL constructor will throw.
There was a problem hiding this comment.
I mean if there is no opts.upstream, jut specify / as the rewritePrefix. We can skip parsing the URL.
There was a problem hiding this comment.
new URL('http://empty').pathname this construction should return /.
Another way to solve this situation is to additionally check upstream and code will be looking like that:
let rewritePrefix = opts.rewritePrefix || opts.upstream ? new URL(opts.upstream).pathname : '/'
There was a problem hiding this comment.
So if the second option is preferred I can update that part.
There was a problem hiding this comment.
I have refactored rewritePrefix as requested.
added support for dynamical upstream configuration
replyOptions.getUpstream must be specified for this
These changes allow using getUpstream feature of fastify-reply-from library to change upstream dynamically