add: error object to custom retry callback

fastify / fastify-reply-from

fastify plugin to forward the current http request to another server

MIT License

148 stars 76 forks source link

add: error object to custom retry callback #337

Closed MikePresman closed 8 months ago

MikePresman commented 8 months ago

This change introduces passing the error object to the custom retry callback; something that should've been done in the initial implementation of this feature but forgotten. Alas this PR introduces passing the error object to the callback.

Checklist

[x] run npm run test and npm run benchmark
[x] tests and/or benchmarks are included
[x] documentation is changed or added
[x] commit message and code follows the Developer's Certification of Origin and the Code of conduct

airhorns commented 8 months ago

@MikePresman want to just include the types PR in this one and adjust the types, and we can merge it together and close the other one?

mcollina commented 8 months ago

There are a lot of CI failures

MikePresman commented 8 months ago

Apologies about the previous failing tests and lint, had pushed and it slipped my mind to check when I grabbed lunch. All should be well now.

Eomm commented 8 months ago

Not a major, this feature is not yet released Here is the discussion https://github.com/fastify/fastify-reply-from/pull/337#discussion_r1432989697

I'm not going to approve as my comment is not solved, feel free to land it if you disagree 👍

MikePresman commented 8 months ago

Not a major, this feature is not yet released Here is the discussion #337 (comment)

I'm not going to approve as my comment is not solved, feel free to land it if you disagree 👍

Could you please clarify, I've made the change to interfacing the function args as an object which is what I believe was the core of your comment unless you meant

The most common use I see:
after 4 retries I want to log the message, otherwise ignore it.

For this reason I think this object will change soonish

passing the logger is something you want in this PR.

gurgunday commented 8 months ago

I'm not going to approve as my comment is not solved, feel free to land it if you disagree 👍

I personally wouldn't merge without making sure that we're all in agreement

Hasn't your proposal been implemented?

MikePresman commented 8 months ago

~Not sure why my above type definition change would have failed the test as no functional changes between HEAD and HEAD~1 should have led to this imo. Could someone kindly re run it? I'm unable to reproduce this on my macos machine.~

Edit: It was reran thank you.

Uzlopak commented 8 months ago

@MikePresman

Did you address all remarks of @airhorns ? If so, could you comment on them accordingly and resolve them please?