HistoryLocation doesn't use baseURL correctly

[jamesarosen]

Ember.HistoryLocation sets baseURL to jQuery('base').attr('href') || ''. That means that baseURL is something like https://mysite.com/root/.

That's totally reasonable, except HistoryLocation#getURL uses baseURL as if it were just a path. That is, it calls (more or less) location.pathname.replace(baseURL, ''). location.pathname will never include the protocol and domain, so that replace will never do anything.

I believe the correct thing to do would be to extract rootURL from baseURL on initialization:

rootURL: null,

init: function() {
  var location = get(this, 'location') || window.location;
  set(this, 'location', location);

  var baseURL = get(this, 'baseURL') || jQuery('base').attr('href') || '';
  set(this, 'baseURL', baseURL);

  var origin = location.origin ||
               "%@//%@%@".fmt(location.protocol, location.hostname, (location.port ? ':' + location.port : ''));
  var rootURL = get(this, 'rootURL') ||
                baseURL.replace(origin, '');
  set(this, 'rootURL', rootURL);
},

Then, only use rootURL in getURL:

getURL: function() {
  var location = get(this, 'location'),
      path = location.pathname,
      baseURL = get(this, 'baseURL');

  baseURL = baseURL.replace(/\/$/, '');
  var url = path.replace(rootURL, '');

  return url;
},

[jamesarosen]

Ah, I think I see the confusion. HistoryLocation is tested with a baseURL of /base/. That's certainly a valid value for the href attribute of the <base> tag.

But it's perfectly sensible for a page to use <base href="https://example.com/base/" />.

And if you ask for $('base').prop('href') (instead of 'attr') or for window.document.baseURI, you always get a full URL with protocol, host, and (if supplied), port.

For example, on this page:

$('<base>').attr('href', '/agent/').prop('href'); // "https://github.com/agent/"

[jamesarosen]

rootURL is also used as a path in formatURL:

formatURL: function(url) {
  // ...
  return baseURL + rootURL + url;
},

That suggests that this might be better called rootPath or, alternatively, that we calculate a single baseURI and throw out rootURL and baseURL altogether.

[jayphelps]

FWIW I agree that rootURL should be renamed rootPath as it's used as such all over ember.

@jamesotron Have you started patching this baseURL issue somewhere? Don't wanna double-effort.

[jamesarosen]

@jayphelps I suspect you meant jamesarosen, not jamesotron.

I haven't started any code yet. Here are some ideas:

1. eliminate rootURL and baseURL; replace them both with a single baseURI (matching document.baseURI)
2. eliminate rootURL; change baseURL to be a full URL, not just a path
3. change rootURL to rootPath; continue to append it to baseURL
4. change rootURL to rootPath; change baseURL to origin (matching window.location.origin); extract both from document.baseURI by default

In any of those cases, we can add deprecation warnings to the existing properties and have them modify the new properties as needed. Basically: the transition isn't hard once we know where we're going.

[jayphelps]

@jamesarosen WHOOPS! I did indeed mean you.

Sorry @jamesotron. Carry on.

[jimsynz]

ha! will do.

[jayphelps]

@jamesarosen I'm neck deep in the location classes and router again so I'm gonna try to squash this issue. Any additional thoughts or revelations since we last discussed or should I plow ahead?

[jamesarosen]

We experimented with a number of options. I think my (4) above is closest to what we ended up with.

IE doesn't have document.baseURI, so you will still have to use $('base').first().attr('href') or something to get it.

[wagenet]

@jayphelps status?

[rwjblue]

@jayphelps / @jamesarosen - Is this still an issue? Any chance one of you would try whipping up a PR?

[jayphelps]

@rwjblue I don't have the cycles to dig in right now 😞

[jamesarosen]

Yes, I can take this on. Our next sprint starts next Monday. I can try to slate it in there.

[jamesarosen]

OK, so my plan is

AutoLocation

• add rootPath
• change rootURL to return rootPath and emit a deprecation warning
• change the rest to use rootPath instead of rootURL

HistoryLocation

• keep baseURL, but use .prop('.href') so it's always a full URL
• add rootPath
• change rootURL to return rootPath and emit a deprecation warning
• change the rest to use rootPath instead of rootURL

Tests

Not quite sure yet.

[sandstrom]

@jamesarosen this would be a great addition, happy when I saw a fix is in the works! ⛵

[locks]

@jamesarosen anything happening here?
cc @rwjblue

[jamesarosen]

I had a PR, but it lingered and was closed. I stopped working on the app that this was a problem for about a year and a half ago.

[locks]

@jamesarosen thanks for the work! I'm closing this issue, sorry the PR didn't get merged at the time.