feat(Worker): add cf-* headers to request #21
Conversation
|
Hi, thanks for the pr. If you can fix the IP address thing I’ll merge it :-). Any thoughts on how to do https? |
|
@gja thanks for the feedback. I don't know if I have this quite right yet. I've added a buildRequest function in server.js and moved all the request building logic in server.js and worker.js to that method. The function takes the express request as it's only argument. It's a tricky refactor because of the dependency shift. Building the request for the fetch event needs the ip, etc. I'm wondering if the worker spec should take a dependency on |
| headers: req.headers, | ||
| function buildRequest(req) { | ||
| const url = `${req.protocol}://${req.get('host')}${req.originalUrl}`; | ||
| const ip = req.headers['x-forwarded-for'] || req.connection.remoteAddress.split(':').pop(); |
There was a problem hiding this comment.
req.connection.remoteAddress.split(':').pop()
pro: no additional dependencies
con: there are more robust options
|
Good points. I think it does make sense to keep the logic in the worker instead of in the server. Primarily because the worker should be more reusable. Is it possible to keep the API signature of worker the same? (taking url and opts). But the Headers are added by the worker. opts.remoteAddress can have the value of node.connection.remoteAddress. Let me know if you'd like me to pick this up as well, I have a bit of bandwidth tonight. |
|
Thanks @gja, I'm happy to keep plugging away at it but if you have bandwidth and want to pick this up, please go ahead. I think you have a clearer vision for how this should be implemented. I'll probably be able to get back to this in the next few days. |
#17