upstash / qstash-js

Message queue for serverless
https://docs.upstash.com/qstash
MIT License
133 stars 11 forks source link

Handle custom header additions without requiring "Upstash-Forward-*" as a prefix. #55

Closed msywulak closed 7 months ago

msywulak commented 9 months ago

Description:

Currently, if you need to send custom headers to a schedule's destination you must prefix them manually with "Upstash-Forward-*". If you forget to add the prefix the headers will be ignored.

Also, deletion did not work until using new parseResponseAsJson property.

Updates

Fixes #52

msywulak commented 9 months ago

Also, if you're alright with the method here or have a cleaner way to do it I could add it to the following as well:

Probably could add in parseResponseAsJson: false, into a couple of places that need it now.

ogzhanolguncu commented 7 months ago

Looks good to me. Since we added failure callback recently this ignoredHeaders[] requires an update. Also let's turn this into some sort of utility since we are going to call this in multiple places. And, finally please add an appropriate example similar to nodejsexample in the example/nodejs repo.

msywulak commented 7 months ago

@ogzhanolguncu I haven't made an example yet but is this what you had in mind?

ogzhanolguncu commented 7 months ago

Util is looking good. What I have in my mind is NodeJS Example adding an another example here where you don't need pass Upstash-Forward-*. Just to showcase people its possible.

Maybe add another file here where you pass optinal headers to your callback.

ogzhanolguncu commented 7 months ago

Also, if you're alright with the method here or have a cleaner way to do it I could add it to the following as well:

  • topics.ts
  • client.ts (publish and publishJSON)

Probably could add in parseResponseAsJson: false, into a couple of places that need it now.

And, let's do this since we now have a easier way to handle this with thanks toprefixHeaders func.

msywulak commented 7 months ago

About to add the others but I was looking through the API docs and couldn't find a place where you can add custom headers on topics. I tested a couple of ways but couldn't make it work - trying to see if it was just undocumented but that doesn't look to be the case.

ogzhanolguncu commented 7 months ago

The PR is looking good right now. However, I have a concern: what if someone adds a new default header to QStash, and we overlook it or forget to include it here? Since it won't be in the predefined list, it could result in rendering the default header as null, potentially causing issues on the servers.

msywulak commented 7 months ago

At the end of the day the client is setting those headers after the util runs, if it was set on the original req, but changed the util up a bit to always ignore any header starting with "upstash-" or "content-type". This would make it so you could never set a Upstash specific header using the headers property, but I think that would likely be expected based on the comment for the header property saying that "these headers will be sent to your destination".

ogzhanolguncu commented 7 months ago

This is looking much better. I'll test it myself too and merge it tomorrow. Good work 👍🏻