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

added --proxy path@url command line argument #157

Open
wants to merge 334 commits into
base: master
Choose a base branch
from

Conversation

talmobi
Copy link

@talmobi talmobi commented Jun 1, 2016

eg: --proxy /api@http://localhost:3000/api

Proxies request to given url from given path (for example when proxying to API server)

working sample:

budo scripts/main.js --live --proxy /api@http://localhost:3030/api -- -t rollupify -t sassr -t babelify

…e/watchify-api

Conflicts:
	README.md
	package.json
@mattdesl
Copy link
Owner

mattdesl commented Jun 2, 2016

Cool, thanks for PR! I've never used proxies, can you give an example of when it would come in handy? How do I test for it? Is that dependency going to add significant bloat to budo?

Cheers 🙌

Sent from my iPhone

On Jun 1, 2016, at 12:33 PM, talmobi notifications@github.com wrote:

eg: --proxy /api@http://localhost:3000/api

Proxies request to given url from given path (for example when proxying to API server)

working sample:

budo scripts/main.js --live --proxy /api@http://localhost:3030/api -- -t rollupify -t sassr -t babelify

You can view, comment on, or merge this pull request online at:

#157

Commit Summary

added --proxy path@url command line argument (eg: --poryx /api@http://localhost:3000/api)
removed semicolon
File Changes

M README.md (1)
M lib/middleware.js (16)
M lib/parse-args.js (3)
M package.json (1)
Patch Links:

https://github.com/mattdesl/budo/pull/157.patch
https://github.com/mattdesl/budo/pull/157.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@quantizor
Copy link

quantizor commented Jun 2, 2016 via email

@talmobi
Copy link
Author

talmobi commented Jun 2, 2016

This is something better solved with a middleware IMO. I actually made one for this express purpose: https://www.npmjs.com/package/middleware-proxy

That's what I did at first but grew bored of always writing a config. A simple --proxy command line is quick and does all that I need.

Cool, thanks for PR! I've never used proxies, can you give an example of when it would come in handy?

If you have any kind of backend or API that your front-end depends on you'd have to write a config to run the budo server in tandem (attach as middleware) with your backend. With the command line you could proxy directly to your backend/api without writing a config.

How do I test for it?

I could write tests for it. Basically could run an express app and see if the proxy works.

Is that dependency going to add significant bloat to budo?

It's a pretty thin lib with full proxy support, <1000LOC, http-proxy depends on;

https://github.com/nodejitsu/node-http-proxy/blob/master/package.json
"dependencies": { "eventemitter3": "1.x.x", "requires-port": "1.x.x" },

..You could move budo's request devDependency to dependencies and write a not really smaller and less complete (like what middleware-proxy does) proxy -- But request is a huge library and would add much more bloat if it were in dependencies.

…ts at test/test-proxy.js, added vim tmp files to .gitignore
@talmobi
Copy link
Author

talmobi commented Jun 2, 2016

I added tests and switched to using opts.proxy so it works like the others like --live. The tests uses a node.js http server as a temporary backend api to test against:

https://github.com/talmobi/budo/blob/master/test/test-proxy.js

@talmobi
Copy link
Author

talmobi commented Jun 2, 2016

Ok so it appears travis passes on all node version except node version 0.8 - probably something to do with the http.createServer... I'll check

[EDIT]: It looks like http-proxy isn't supported on node v0.8 - is node v0.8 support important for anyone still?

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.

None yet