Proxy Requests for Unknown Paths #56

sideshowcoder / canned

Server to respond with fake API responses, by using a directory of files for finding out what to say!

213 stars 46 forks source link

Proxy Requests for Unknown Paths #56 #84

Open git-jiby-me opened 8 years ago

git-jiby-me commented 8 years ago

can define proxy in 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

git-jiby-me commented 8 years ago

@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

sideshowcoder commented 8 years ago

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 :)

git-jiby-me commented 8 years ago

@sideshowcoder no problemo

sideshowcoder commented 8 years ago

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?

git-jiby-me commented 8 years ago

@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.

sideshowcoder commented 8 years ago

@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.

git-jiby-me commented 8 years ago

@sideshowcoder i am ok to go with the small changes, i will have them pushed by this week end :+1:

sideshowcoder commented 8 years ago

Awesome!

git-jiby-me commented 8 years ago

@sideshowcoder i will add documentation some day soon, within this week

git-jiby-me commented 8 years ago

@sideshowcoder added documentation as well, have a look and let me know, if its good to merge

sideshowcoder commented 8 years ago

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!

git-jiby-me commented 8 years ago

@sideshowcoder i kind of lost on this because of work, will get this done next week

buzzdecafe commented 7 years ago

bumping this PR since i could really use this feature. please lmk if there is any way i can help.

sideshowcoder commented 7 years ago

@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.

buzzdecafe commented 7 years ago

@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.