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

[Bugfix] fixes bugs around baseURL and rootURL in HistoryLocation and introduces rootURL to NoneLocation #13520

Merged
merged 1 commit into from
May 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/ember-application/lib/system/application-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,13 @@ const ApplicationInstance = EngineInstance.extend({
}
};

let location = get(router, 'location');

// Keeps the location adapter's internal URL in-sync
get(router, 'location').setURL(url);
location.setURL(url);

return router.handleURL(url).then(handleResolve, handleReject);
// getURL returns the set url with the rootURL stripped off
return router.handleURL(location.getURL()).then(handleResolve, handleReject);
}
});

Expand Down
6 changes: 4 additions & 2 deletions packages/ember-routing/lib/location/history_location.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ export default EmberObject.extend({
rootURL = rootURL.replace(/\/$/, '');
baseURL = baseURL.replace(/\/$/, '');

// remove baseURL and rootURL from path
var url = path.replace(baseURL, '').replace(rootURL, '');
// remove baseURL and rootURL from start of path
var url = path
.replace(new RegExp('^' + baseURL), '')
.replace(new RegExp('^' + rootURL), '');

var search = location.search || '';
url += search;
Expand Down
40 changes: 34 additions & 6 deletions packages/ember-routing/lib/location/none_location.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { assert } from 'ember-metal/debug';
import { get } from 'ember-metal/property_get';
import { set } from 'ember-metal/property_set';
import EmberObject from 'ember-runtime/system/object';
Expand All @@ -22,15 +23,38 @@ export default EmberObject.extend({
implementation: 'none',
path: '',

detect() {
let rootURL = this.rootURL;

assert('rootURL must end with a trailing forward slash e.g. "/app/"',
rootURL.charAt(rootURL.length - 1) === '/');
},

/**
Will be pre-pended to path.

@private
@property rootURL
@default '/'
*/
rootURL: '/',

/**
Returns the current path.
Returns the current path without `rootURL`.

@private
@method getURL
@return {String} path
*/
getURL() {
return get(this, 'path');
let path = get(this, 'path');
let rootURL = get(this, 'rootURL');

// remove trailing slashes if they exists
rootURL = rootURL.replace(/\/$/, '');
Copy link
Member

@rwjblue rwjblue May 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we store the normalized rootURL, instead of removing the trailing slash on every getURL / formatURL? Perhaps via adding something like:

_normalizedRootURL() {
  if (this._rootURL) { return this._rootURL; }
  let rootURL = get(this, 'rootURL');

  return this._rootURL = rootURL.replace(/\/$/, '');
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

@jasonmit jasonmit May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caveat this introduces is you wouldn't be able to change the rootURL after the router setup. Unsure if that's an actual use case or not though, I would say it's not but I don't know all the crazy ways people use rootURL :) History location is also doing this as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. We could make it a CP with a setter maybe, but I'm not sure it's worth the effort.


// remove rootURL from url
return path.replace(new RegExp('^' + rootURL), '');
},

/**
Expand Down Expand Up @@ -83,9 +107,13 @@ export default EmberObject.extend({
@return {String} url
*/
formatURL(url) {
// The return value is not overly meaningful, but we do not want to throw
// errors when test code renders templates containing {{action href=true}}
// helpers.
return url;
let rootURL = get(this, 'rootURL');

if (url !== '') {
// remove trailing slashes if they exists
rootURL = rootURL.replace(/\/$/, '');
}

return rootURL + url;
}
});
34 changes: 34 additions & 0 deletions packages/ember-routing/tests/location/history_location_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,40 @@ QUnit.test('HistoryLocation.getURL() returns the current url, excluding both roo
equal(location.getURL(), '/foo/bar');
});

QUnit.test('HistoryLocation.getURL() returns the current url, does not remove rootURL if its not at start of url', function() {
expect(1);

HistoryTestLocation.reopen({
init() {
this._super(...arguments);

set(this, 'location', mockBrowserLocation('/foo/bar/baz'));
set(this, 'rootURL', '/bar/');
}
});

createLocation();

equal(location.getURL(), '/foo/bar/baz');
});

QUnit.test('HistoryLocation.getURL() returns the current url, does not remove baseURL if its not at start of url', function() {
expect(1);

HistoryTestLocation.reopen({
init() {
this._super(...arguments);

set(this, 'location', mockBrowserLocation('/foo/bar/baz'));
set(this, 'baseURL', '/bar/');
}
});

createLocation();

equal(location.getURL(), '/foo/bar/baz');
});

QUnit.test('HistoryLocation.getURL() includes location.search', function() {
expect(1);

Expand Down
69 changes: 69 additions & 0 deletions packages/ember-routing/tests/location/none_location_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { set } from 'ember-metal/property_set';
import run from 'ember-metal/run_loop';
import NoneLocation from 'ember-routing/location/none_location';

var NoneTestLocation, location;

function createLocation(options) {
if (!options) { options = {}; }
location = NoneTestLocation.create(options);
}

QUnit.module('Ember.NoneLocation', {
setup() {
NoneTestLocation = NoneLocation.extend({});
},

teardown() {
run(function() {
if (location) { location.destroy(); }
});
}
});

QUnit.test('NoneLocation.formatURL() returns the current url always appending rootURL', function() {
expect(1);

NoneTestLocation.reopen({
init() {
this._super(...arguments);
set(this, 'rootURL', '/en/');
}
});

createLocation();

equal(location.formatURL('/foo/bar'), '/en/foo/bar');
});

QUnit.test('NoneLocation.getURL() returns the current path minus rootURL', function() {
expect(1);

NoneTestLocation.reopen({
init() {
this._super(...arguments);
set(this, 'rootURL', '/foo/');
set(this, 'path', '/foo/bar');
}
});

createLocation();

equal(location.getURL(), '/bar');
});

QUnit.test('NonoLocation.getURL() will remove the rootURL only from the beginning of a url', function() {
expect(1);

NoneTestLocation.reopen({
init() {
this._super(...arguments);
set(this, 'rootURL', '/bar/');
set(this, 'path', '/foo/bar/baz');
}
});

createLocation();

equal(location.getURL(), '/foo/bar/baz');
});