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

make the user request and response available in skipToNextHandlerFilter #409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raphaeleidus
Copy link
Contributor

update the api of skipToNextHandlerFilter to be function(proxyRes, userReq, userRes) {}

I have a use case where I need to skip the proxy but not based on the response of the proxy.

This change is backwards compatible with the existing api

@monkpow
Copy link
Collaborator

monkpow commented Mar 11, 2019

Hey @raphaeleidus, so I can understand what we're solving for here, would you describe your use case in a little more detail? Specifically, curious why you engage the proxy function.

Overall, the code looks good and I appreciate the attention to tests.

@raphaeleidus
Copy link
Contributor Author

raphaeleidus commented Mar 11, 2019

Yeah sure.

So I have a legacy application that I am progressively migrating away from. Currently my new application knows how to render and respond to some requests but it doesn't know based on URL structure only after looking up the object in the database.

So currently the my two varieties of request handling flows look like this:

|-lookup entity-|-respond from new app-|

Or

|-lookup entity-|-proxy to legacy app-|

Unfortunately for my end user the lookup can be slow and isn't actually needed for the proxy to succeed. So I would like to do the two steps in parallel

edit: will make a diagram to make this clearer

@raphaeleidus
Copy link
Contributor Author

raphaeleidus commented Mar 11, 2019

So here is a flow diagram. Basically as it stands now I have to do the DB lookup before I make the proxy request. I would like to do them concurrently and discard the proxy response if I dont need it.
proxy flow

@monkpow
Copy link
Collaborator

monkpow commented Mar 14, 2019 via email

@monkpow
Copy link
Collaborator

monkpow commented Mar 15, 2019

@raphaeleidus Many thanks for your detailed discussion so far on this. I want to solve for this use case, but I'm conservative about expanding args, and I'm not sure yet that this is the solution I want.

I'd be really happy if you want to talk this through with me.

This solution makes this method more muscular, and gives an implementer more powerful choices about what to do inside the skipToNext handler. That's a very valid solution, and it works right away to solve your use case, which is valuable.

As a general design goal, I keep the steps low wattage on purpose, and give each step a specific and limited role. Part of this is limiting arguments to the narrowest possible scope. In a way, this solution expands the purpose and surface area of skipToNextHandler, and that's what I feel conservative about. I want to see if there's a way to define the problem a little differently before expanding the signature.

I'd prefer a solution that lets express-http-proxy respond with enough information for your app to make a smart decision, with a similar latency profile. I tend to think pushing the information out enables more powerful solutions than bringing the decisions in, if that makes sense.

Is there a signal that express-http-proxy could emit that would help your application decide in its next handler how to resolve the request? skipToNextHandler does attach the proxy.res and proxy.req to the user.res object. I think for your use case, you could bail here, and in the failure case (use the legacy app) you'd be find, but your success case would be broken because you skip the other workflow steps. (let me know if I got this right).

Maybe skip should be later? Maybe it actually an additional workflow step? Maybe its what
https://github.com/villadora/express-http-proxy/blob/master/app/steps/sendUserRes.js
should do?

I'm just spitballing for discussion right now; it might resolve to what you've already implemented. I'm interested in your thoughts about this if you're game to discuss.

Thanks again.

@raphaeleidus
Copy link
Contributor Author

Hey @monkpow I would love to discuss this more I do have an idea or two here of some other patterns. I am away on vacation right now but maybe we can do a quick call and talk through some stuff next week if you are up for it. I am in UTC-4 timezone (NYC)

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

Successfully merging this pull request may close these issues.

2 participants