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

onResponse res instead of res.stream #334

Closed
2 tasks done
Swizz opened this issue Dec 13, 2023 · 5 comments
Closed
2 tasks done

onResponse res instead of res.stream #334

Swizz opened this issue Dec 13, 2023 · 5 comments

Comments

@Swizz
Copy link

Swizz commented Dec 13, 2023

Prerequisites

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

🚀 Feature Proposal

onReponse method provide access only to the res.stream but we are facing a scénario where it might be useful to use the whole res instead.

And we found no means to get access to it.
To do not be a breaking change, onReponse could provide both res.stream and res.

Motivation

Hi,
thank you for bringing us fastify-reply-from.

We using it to simplify our API for end customers. But in some scenario we have to merge two APIs in one because we have a lot of multi steps APIs.

Be used fastify-reply-from so far with some little tweaks to get access to the response of the step 1 for passing it to the step 2.

But in one of our scenario an API reply with a 204 that is not an Error, but result in a complete end of the original request instead of moving to the step 2. We are able to override the reply status code, but we would like a way to know if it is a 204 originally or any other codes.

We understand that reply-from main goal is to proxy only one request. But in our case, it would be useful to have access to the whole response as well.

Cheers,

Example

Instead of onResponse(request, reply, resBody) it could be onResponse(request, reply, resBody, res)

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 13, 2023

I assume that res.stream is used for higher throughput. Maybe we should implement an option to select if res or stream is passed through.
This would empower others to get what they need.

@mcollina
Copy link
Member

Given that we had a regression for 204 recently, could you include a reproduction for the 204 problem you are mentioning?

I'm not sure I understand your use case. I think your code would be easier to read and maintain if you just used undici.request directly to orchestrate your APIs?

@Swizz
Copy link
Author

Swizz commented Jan 3, 2024

Hi, thanks for your inquiry.

Here is a repo that try to reproduce our use case : Swizz/reply-from-dummy
Because of a legacy codebase, we are still using node 14 and fastify v2 in production.

By running npm run start, you will be provided a swagger interface at /docs.
All routes are described in the readme, and more comments in the code will enlight you about what we are trying to achieve. The special routes /post/100 will mimic the 204 behavior.

We tried to use undici, but you did a great job on reply-from to relay directly headers such as authentication or request status for errored routes that saved us for a lot of headaches.

But we would totally understand if your are against such changes because opening to more features could add you maintenance on subjects your package was not intented for.

@mcollina
Copy link
Member

mcollina commented Jan 6, 2024

I'm extremely surprised that the latest version of this module works with such an old version of Fastify.

Composing multiple endpoint into out is somewhat out of the one of @fastify/reply-from. I'd recommend to use undici directly and forward the headers.

If you are using OpenAPI, you can check out the automated client we created in Platformatic. It has facilities to forward headers etc.

https://docs.platformatic.dev/docs/reference/client/introduction/#openapi

If you need professional help with the migration or anything similar, feel free to ping me via email, happy to have a chat.

@Swizz
Copy link
Author

Swizz commented Jan 16, 2024

Thank you for your heads up. We will try to move on undici then.

I think we can close this one, then.

Thanks a lot for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants