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

Port should default to window.location.port, not 80, if not specified #60

Open
MadMartian opened this issue Oct 18, 2013 · 4 comments
Open

Comments

@MadMartian
Copy link

The port should not default to port 80 if not specified in a request. Instead it should default to the value obtained from window.location.port unless it is undefined in which case it should default to 80.

This snippet:

port: {
    get: function() {
      if (!this._port) {
        switch(this.scheme) {
          case "https": return this._port = 443;
          case "http":
          default: return this._port = 80;
        }
      }
      return this._port;
    },
    set: function(value) { this._port = value; return this; },
    enumerable: true
  },

Conflicts with this snippet:

http.request = function (params, cb) {
    if (!params) params = {};
    if (!params.host) params.host = window.location.host.split(':')[0];
    if (!params.port) params.port = window.location.port;

    var req = new Request(new xhrHttp, params);
    if (cb) req.on('response', cb);
    return req;
};

When issuing a request using a relative path that omits the scheme, host, and port. The library correctly obtains the host from window.location.host, but does not correctly obtain the port from window.location.port.

@tmattsson
Copy link

Also ran into this one, thanks for the fix @MadMartian

@automatthew
Copy link
Contributor

Probable fix for this is now in master. I don't have browser tests set up at the moment, so I won't publish a release yet.

@dyoder
Copy link
Member

dyoder commented May 17, 2014

Leaving this open for now, as part of the NG release plan. We'll use this fix as a model. See also: #71, #72.

@nfriedly
Copy link

nfriedly commented Jun 1, 2015

BTW, I think I see a related bug looking at the current master - https://github.com/pandastrike/shred/blob/master/src/request.coffee#L86 is defaulting to window?.port, but I think that should be location?.port or window?.location?.port (equivalent, but the latter is more obvious if you're not thinking about a client-side context)

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

No branches or pull requests

5 participants