sideshowcoder
Proxy Requests for Unknown Paths #56
- can define
proxyin options as a domain to forward requests with unknown path to - requests with unknown path will be forwarded to proxy domain, and response provided as mock
@sideshowcoder everything works now, take a look at this, let me know if you have some comments. I need you help documenting this into README
Thanks for all your work :) I will look through it as soon as possible not gonna make it today but likely tomorrow. I have to think about a little I guess but first look thorugh looks very good :)
Looks very good! One thing I would suggest as a refactor is moving the proxy response into its own object, we already have the Response which gets returned and send back, maybe making a compatible ProxyResponse object which behaves the same way but uses the Proxy underneath would remove some of the more complicated pieces out of the main and would make them even more easily testable. What do you think?
@sideshowcoder well it would definitely makes sense, and i also like to do it, but i would propose to have a refactor PR, problems i see currently are
- so much logic inside canned.js, we should split it up.
- i don't see the current logic of reading all mock files each time a request comes up, we should read on init, and when something changes in the folder, would make it a lot cleaner, and will make it possible to better separate code.
I see that you already got a proposal to use promises, and you found its pros to not weight in for a change, i want to have another try at it, so lets keep this PR paused, and let me work on a proposal for refactor and then come back to this PR ?
Do you agree with my idea, because i would really like to make the code organised better and performant, before we do much stuff on additional features, i could make a spec, and then we can work our way up.
@git-jiby-me I'm happy to have this PR in with the small changes according to the comments if you are alright with that. Than we can refactor towards a better split. I fear that if we keep this on hold much is going to move and this is not going to be mergable anymore soon. What do you think about making the small changes + README and merge this and open a 2. PR for the refactor? Or are we already agreeing on this and I missread, if so sorry.
@sideshowcoder i am ok to go with the small changes, i will have them pushed by this week end :+1:
@sideshowcoder i will add documentation some day soon, within this week
@sideshowcoder added documentation as well, have a look and let me know, if its good to merge
Just looked through it sorry for the slow response but it was a busy weekend. I think this now looks good to me! You might want to squash the commits before merging (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html) the workflow would be like
$ git rebase -i master
... change pick to squash for all but the first commit
... write a good commit message to describe the whole feature
$ git push origin feature-56_proxy_request -f
now when you reload the PR this should only have one commit in it. As there are many commits in this PR it makes it a little cleaner. If you need help feel free reach out and I can help! Thank you so much for all the work this has been on my list for long and it is a really nice implementation!
@sideshowcoder i kind of lost on this because of work, will get this done next week
bumping this PR since i could really use this feature. please lmk if there is any way i can help.
@buzzdecafe I guess the thing left is to confirm it works, rebase against master and thats it. I'm happy to merge probably after the weekend. I can take a look on monday but help is much appriciated to a) confirm it works, and b) rebase with current master.
@sideshowcoder After rebasing master onto that branch I am getting 71/73 test failures. :smile: Looks like there have been some API changes since this PR. So no, it doesn't work yet.