Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retriesCount not defined in interface FastifyReplyFromHooks #296

Open
2 tasks done
MaximeCheramy opened this issue Mar 6, 2023 · 2 comments
Open
2 tasks done

retriesCount not defined in interface FastifyReplyFromHooks #296

MaximeCheramy opened this issue Mar 6, 2023 · 2 comments
Labels

Comments

@MaximeCheramy
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.14.0

Plugin version

8.4.3

Node.js version

18

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Ubuntu 22.04

Description

It seems possible to configure the number of retries according to the documentation and the code (https://github.com/fastify/fastify-reply-from/blob/master/README.md)

However, in the types I don't see anything related to retriesCount. Is it simply missing or did I misunderstood how to configure it?

Steps to Reproduce

Try to configure retriesCount in TypeScript.

Expected Behavior

No response

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2023

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

@mcollina
Copy link
Member

mcollina commented Mar 7, 2023

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.

@gurgunday gurgunday added the types label Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants