retriesCount not defined in interface FastifyReplyFromHooks #296

Activity

1. Uzlopak commented on Mar 7, 2023

   

   Uzlopak

   on Mar 7, 2023

   Last edited by Uzlopak

   Contributor

   More actions

   It should be defined in FastifyReplyFromOptions but is missing there.

   I am also wondering if it is implemented correct. I would expect that if i set options on registering the plugin than that they become the default settings for the actual reply.from call.

   Current codebase is like this

   const disableRequestLogging = opts.disableRequestLogging || false
   
     fastify.decorateReply('from', function (source, opts) {
       opts = opts || {}
       const req = this.request.raw
       const onResponse = opts.onResponse
       const rewriteHeaders = opts.rewriteHeaders || headersNoOp
       const rewriteRequestHeaders = opts.rewriteRequestHeaders || requestHeadersNoOp
       const getUpstream = opts.getUpstream || upstreamNoOp
       const onError = opts.onError || onErrorDefault
       const retriesCount = opts.retriesCount || 0
       const maxRetriesOn503 = opts.maxRetriesOn503 || 10
   
   // And so on

   I would have expected something like this:

   const disableRequestLogging = opts.disableRequestLogging || false
   const defaultOnResponse = opts.onResponse
   const defaultRewriteHeaders = opts.onResonse || headersNoOp
   const defaultGetUpstream = opts.getUpstream || upstreamNoOp
   const defaultOnError = opts.onError || onErrorDefault
   const defaultRetriesCount = opts.retriesCount || 0
   const defaultMaxRetriesOn503 = opts.maxRetriesOn503 ?? 10
   
     fastify.decorateReply('from', function (source, opts) {
       opts || (opts = {})
       const req = this.request.raw
       const onResponse = opts.onResponse || defaultOnResponse
       const rewriteHeaders = opts.rewriteHeaders || defaultRewriteHeaders
       const rewriteRequestHeaders = opts.rewriteRequestHeaders || requestHeadersNoOp
       const getUpstream = opts.getUpstream || defaultGetUpstream
       const onError = opts.onError || defaultOnError
       const retriesCount = opts.retriesCount ?? defaultRetriesCount
       const maxRetriesOn503 = opts.maxRetriesOn503 ?? defaultMaxRetriesOn503
   
   // And so on

   In the current code you can not even set maxRetriesOn503 to 0 because of using || and not ??

   I typed this comment with my phone so there can be typos in the code.

   @mcollina
   @climba03003
2. mcollina commented on Mar 7, 2023

   

   mcollina

   on Mar 7, 2023

   Member

   More actions

   if the implementation is not correct, I would love to see a test/repro to verify the problem.

   ---

   @MaximeCheramy would you like to send a PR to add retriesCount to the types? I think it should go to the @fastify/reply-from module.
3. gurgunday

   added

   types

   on Apr 4, 2024