Remove the proxy-host and proxy-port, introduce proxy-url instead #567

petrjanda · 2014-05-02T10:46:18Z

Simplified proxy setup, fixes #465.

petrjanda · 2014-05-02T10:54:53Z

One thing I am wondering though is that I was tempted to use it as:

ember server --proxy-url localhost:3000

Which wouldn't work as url.parse will think 'localhost' is a protocol (http://nodejs.org/api/url.html#url_url_parse_urlstr_parsequerystring_slashesdenotehost).

Proxy will still kick off if I skip the protocol but later result in error, while trying to proxy:

TypeError: Cannot read property 'length' of null
    at slashJoin (/Users/petr/experimental/ember/ember-cli/node_modules/proxy-middleware/index.js:83:9)
    at Object.handle (/Users/petr/experimental/ember/ember-cli/node_modules/proxy-middleware/index.js:24:17)
    at next (/Users/petr/experimental/ember/ember-cli/node_modules/express/node_modules/connect/lib/proto.js:193:15)
    at SendStream.<anonymous> (/Users/petr/experimental/ember/ember-cli/node_modules/broccoli/lib/middleware.js:17:13)
    at SendStream.EventEmitter.emit (events.js:95:17)
    at SendStream.error (/Users/petr/experimental/ember/ember-cli/node_modules/broccoli/node_modules/send/lib/send.js:147:51)
    at SendStream.onStatError (/Users/petr/experimental/ember/ember-cli/node_modules/broccoli/node_modules/send/lib/send.js:248:48)
    at /Users/petr/experimental/ember/ember-cli/node_modules/broccoli/node_modules/send/lib/send.js:320:26
    at Object.oncomplete (fs.js:107:15)

ember server --proxy-url http://localhost:3000

is valid usage.

MajorBreakfast · 2014-05-02T11:00:25Z

Jep you should append http:// automatically if it wasn't specified. The shorter it is the more convenient. Also consider correcting the url to http://localhost:4875 for 4875 (Just a port number)

Sry for closing and reopening this. I've hit the button accidentally.

stefanpenner · 2014-05-02T14:40:35Z

lib/commands/serve.js

@@ -10,8 +10,7 @@ module.exports = Command.extend({
  availableOptions: [
    { name: 'port', type: Number, default: 4200 },
    { name: 'host', type: String, default: '0.0.0.0' },
-    { name: 'proxy-port',  type: Number },
-    { name: 'proxy-host',  type: String },
+    { name: 'proxy-url',  type: String },


why not just --proxy ?

Although --proxy works, too (because nopt expands it), I also think we could just call it --proxy. It's simpler.

Yeah, sounds good.

stefanpenner · 2014-05-02T14:41:07Z

i like this approach, can you add some tests, and an entry in the changelog?

stefanpenner · 2014-05-02T15:13:21Z

@MajorBreakfast im not concerned about length, this will most commonly be driven from the dotfile

petrjanda · 2014-05-02T15:22:23Z

To me url.parse('localhost:3000') resulting in:

{ protocol: 'localhost:',
  slashes: null,
  auth: null,
  host: '3000',
  port: null,
  hostname: '3000',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'localhost:3000' }

itself does feel wrong. I've asked node.js team about it (nodejs/node-v0.x-archive#7547). Eventually if url.parse would behave more as expected (initializing host/port), it would be easier to plug in defaults after url is parsed, rather then fiddling with defaults on strings level.

petrjanda · 2014-05-02T15:27:10Z

@stefanpenner Yeah couldn't find any proxy related tests so can add some. I am not too familiar with the codebase though. To what level (integration/unit) you want to test it?

MajorBreakfast · 2014-05-02T15:28:25Z

@stefanpenner I think calling proxy should be fine. I don't think that we'll get another proxy option.
@petrjanda That's not really a bug. url.parse expects a valid url as input. If it doesn't get one its behaviour is kind of undefined. Either way we shouldn't depend on it.

if (/^\d*$/.test(options.proxy)) { proxy = 'http://localhost:' + proxy }
if (!/^https?:\/\//.test(options.proxy)) { proxy = 'http://' + proxy }

petrjanda · 2014-05-02T15:37:59Z

@MajorBreakfast Yeah, that would certainly work.

Alternative approach might be to expect the ember-cliuser to provide the valid url for proxying as well. What feels wrong is to let it go through so he would run into issues I described above.

So yeah, solution injecting defaults or better error feedback both look good to me. If going for defaults, should it be somewhere explicitly mentioned or we expect 'http://localhost:' as implicit "reasonable" default?

stefanpenner · 2014-05-03T18:07:02Z

@petrjanda status? Would love to get this in for this weekends release.

petrjanda · 2014-05-04T11:19:20Z

@stefanpenner couldn't figure out nice way to test it. Suggestions?

What about the defaults (protocol / domain)? Shall we add them as well?

stefanpenner · 2014-05-04T14:17:52Z

I would prefer we only allowed the fully qualified uri for now. No short cuts

stefanpenner · 2014-05-04T23:25:35Z

@petrjanda like a test in: https://github.com/stefanpenner/ember-cli/blob/master/tests/unit/cli/cli-test.js to ensure the param gets passed through.

And another test to ensure the serve task when given the URI passes it through to the server

petrjanda · 2014-05-05T07:04:46Z

tests/unit/cli/cli-test.js

@@ -125,6 +125,17 @@ describe('Unit: CLI', function() {

  describe('server', function() {
    ['server','s'].forEach(function(command) {
+      it('expects version in UI output', function() {


Refactored this to separate test. I wanted to increase output.length in assert as proxy does write extra line to the output but because of stub the server command is actually never called in this spec so was rather confusing. Afaik only output it handles is before command.validateAndRun is triggered.

petrjanda · 2014-05-05T07:33:20Z

tests/unit/tasks/server/express-server-test.js

+  it('should start proxy if ```proxy``` URI is given', function() {
+    expressServer.ui = new MockUI();
+    return expressServer.start({proxy: 'http://localhost:3000/'}).then(function() {
+      var output = expressServer.ui.output.trim().split('\n');


Not really best test. I cant use stub test helper for constructors, right? I think stubbing and testing the call to proxy would be ideal.

you could also match against the output make sure it contains the proxy URI

stefanpenner · 2014-05-05T19:28:09Z

this is looking pretty good, let me know when its ready. (it will also need a rebase)

abuiles · 2014-05-06T02:57:01Z

tests/unit/commands/server-test.js

+
+    assert.equal(serveRun.called, 1, 'expected run to be called once');
+
+    assert.equal(options.proxy, 'http://localhost:3000/', 'has correct port');


has correct proxy?

stefanpenner · 2014-05-06T03:50:35Z

merged manually f672ede

ndreckshage · 2014-05-07T16:34:13Z

this is great. thanks, a lot better than launching chrome without web security

remove the proxy-host and proxy-port, introduce proxy-url instead

ea18695

MajorBreakfast closed this May 2, 2014

MajorBreakfast reopened this May 2, 2014

stefanpenner reviewed May 2, 2014
View reviewed changes

petrjanda added 3 commits May 4, 2014 12:12

rename proxy-url to proxy

1a3e0f9

add changelog entry

e871214

fix changelog entry

64249b4

petrjanda added 2 commits May 5, 2014 07:49

add cli param test

d95d386

remove duplicate assertions

3b366f5

petrjanda reviewed May 5, 2014
View reviewed changes

add serve proxy test, add express server test

3538f11

petrjanda reviewed May 5, 2014
View reviewed changes

abuiles reviewed May 6, 2014
View reviewed changes

abuiles mentioned this pull request May 6, 2014

Update mention to proxy. #612

Merged

stefanpenner closed this May 6, 2014

rwjblue mentioned this pull request May 7, 2014

Usage of proxy-host #465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the proxy-host and proxy-port, introduce proxy-url instead #567

Remove the proxy-host and proxy-port, introduce proxy-url instead #567

petrjanda commented May 2, 2014

petrjanda commented May 2, 2014

MajorBreakfast commented May 2, 2014

stefanpenner May 2, 2014

MajorBreakfast May 2, 2014

petrjanda May 2, 2014

stefanpenner commented May 2, 2014

stefanpenner commented May 2, 2014

petrjanda commented May 2, 2014

petrjanda commented May 2, 2014

MajorBreakfast commented May 2, 2014

petrjanda commented May 2, 2014

stefanpenner commented May 3, 2014

petrjanda commented May 4, 2014

stefanpenner commented May 4, 2014

stefanpenner commented May 4, 2014

petrjanda May 5, 2014

petrjanda May 5, 2014

stefanpenner May 5, 2014

petrjanda May 5, 2014

stefanpenner commented May 5, 2014

abuiles May 6, 2014

stefanpenner commented May 6, 2014

ndreckshage commented May 7, 2014


		assert.equal(serveRun.called, 1, 'expected run to be called once');

		assert.equal(options.proxy, 'http://localhost:3000/', 'has correct port');

Remove the proxy-host and proxy-port, introduce proxy-url instead #567

Remove the proxy-host and proxy-port, introduce proxy-url instead #567

Conversation

petrjanda commented May 2, 2014

petrjanda commented May 2, 2014

MajorBreakfast commented May 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented May 2, 2014

stefanpenner commented May 2, 2014

petrjanda commented May 2, 2014

petrjanda commented May 2, 2014

MajorBreakfast commented May 2, 2014

petrjanda commented May 2, 2014

stefanpenner commented May 3, 2014

petrjanda commented May 4, 2014

stefanpenner commented May 4, 2014

stefanpenner commented May 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented May 5, 2014

Choose a reason for hiding this comment

stefanpenner commented May 6, 2014

ndreckshage commented May 7, 2014