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

Fix: Router ignored the root when creating hrefs #601

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
8 changes: 6 additions & 2 deletions src/navigation-commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,12 @@ export class Redirect {
* @param appRouter The router to be redirected.
*/
navigate(appRouter: Router): void {
let navigatingRouter = this.options.useAppRouter ? appRouter : (this.router || appRouter);
navigatingRouter.navigate(this.url, this.options);
if (this.options.useHistory) {
appRouter.history.navigate(this.url, this.options);
} else {
let navigatingRouter = this.options.useAppRouter ? appRouter : (this.router || appRouter);
navigatingRouter.navigate(this.url, this.options);
}
}
}

Expand Down
19 changes: 16 additions & 3 deletions src/navigation-plan.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,17 @@ export function _buildNavigationPlan(instruction: NavigationInstruction, forceLi

if ('redirect' in config) {
let router = instruction.router;
return router._createNavigationInstruction(config.redirect)
return router._createNavigationInstruction(config.redirect, instruction.parentInstruction)
.then(newInstruction => {
let params = Object.keys(newInstruction.params).length ? instruction.params : {};
let redirectLocation = router.generate(newInstruction.config.name, params, instruction.options);
const baseUrl = getInstructionBaseUrl(instruction);
let redirectLocation = router.generate(newInstruction.config.name, params, instruction.options, baseUrl);

if (instruction.queryString) {
redirectLocation += '?' + instruction.queryString;
}

return Promise.resolve(new Redirect(redirectLocation));
return Promise.resolve(new Redirect(redirectLocation, {useHistory: true}));
});
}

Expand Down Expand Up @@ -111,6 +112,18 @@ export function _buildNavigationPlan(instruction: NavigationInstruction, forceLi
return Promise.resolve(plan);
}

function getInstructionBaseUrl(instruction) {
const instructionBaseUrlParts = [];
instruction = instruction.parentInstruction;

while (instruction) {
instructionBaseUrlParts.unshift(instruction.getBaseUrl());
instruction = instruction.parentInstruction;
}

return instructionBaseUrlParts.join('');
}

function hasDifferentParameterValues(prev: NavigationInstruction, next: NavigationInstruction): boolean {
let prevParams = prev.params;
let nextParams = next.params;
Expand Down
27 changes: 20 additions & 7 deletions src/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ export class Router {
}

this.isExplicitNavigation = true;
return this.history.navigate(_resolveUrl(fragment, this.baseUrl, this.history._hasPushState), options);
return this.history.navigate(_resolveUrl(fragment, this.baseUrl, this.history._hasPushState, this.history.root), options);
}

/**
Expand All @@ -237,7 +237,7 @@ export class Router {
*/
navigateToRoute(route: string, params?: any, options?: any): NavigationResult {
let path = this.generate(route, params);
return this.navigate(path, options);
return this.history.navigate(path, options);
}

/**
Expand Down Expand Up @@ -268,7 +268,7 @@ export class Router {
* @param options If options.absolute = true, then absolute url will be generated; otherwise, it will be relative url.
* @returns {string} A string containing the generated URL fragment.
*/
generate(name: string, params?: any, options?: any = {}): string {
generate(name: string, params?: any, options?: any = {}, baseUrlOverride: string = null): string {
let hasRoute = this._recognizer.hasRoute(name);
if ((!this.isConfigured || !hasRoute) && this.parent) {
return this.parent.generate(name, params);
Expand All @@ -279,8 +279,21 @@ export class Router {
}

let path = this._recognizer.generate(name, params);
let rootedPath = _createRootedPath(path, this.baseUrl, this.history._hasPushState, options.absolute);
return options.absolute ? `${this.history.getAbsoluteRoot()}${rootedPath}` : rootedPath;
let rootedPath = _createRootedPath(path, baseUrlOverride ? baseUrlOverride : this.baseUrl, this.history._hasPushState, this.history.root);
if (options.absolute) {
// Hamfisted workaround because history does not expose the origin without the root
let origin = this.history.getAbsoluteRoot();
if (this.history._hasPushState) {
if (this.history.root) {
origin = origin.substring(0, origin.length - this.history.root.length);
} else {
origin = origin.substring(0, origin.length - 1);
}
}

rootedPath = origin + rootedPath;
}
return rootedPath;
}

/**
Expand Down Expand Up @@ -431,9 +444,9 @@ export class Router {
for (let i = 0, length = nav.length; i < length; i++) {
let current = nav[i];
if (!current.config.href) {
current.href = _createRootedPath(current.relativeHref, this.baseUrl, this.history._hasPushState);
current.href = _createRootedPath(current.relativeHref, this.baseUrl, this.history._hasPushState, this.history.root);
} else {
current.href = _normalizeAbsolutePath(current.config.href, this.history._hasPushState);
current.href = _normalizeAbsolutePath(current.config.href, this.history._hasPushState, this.history.root);
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions src/util.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
export function _normalizeAbsolutePath(path, hasPushState, absolute = false) {
export function _normalizeAbsolutePath(path, hasPushState, root) {
if (!hasPushState && path[0] !== '#') {
path = '#' + path;
}

if (hasPushState && absolute) {
path = path.substring(1, path.length);
if (hasPushState && root) {
path = root + path.substring(1, path.length);
}

return path;
}

export function _createRootedPath(fragment, baseUrl, hasPushState, absolute) {
export function _createRootedPath(fragment, baseUrl, hasPushState, root) {
if (isAbsoluteUrl.test(fragment)) {
return fragment;
}
Expand All @@ -31,15 +31,15 @@ export function _createRootedPath(fragment, baseUrl, hasPushState, absolute) {
path = path.substring(0, path.length - 1);
}

return _normalizeAbsolutePath(path + fragment, hasPushState, absolute);
return _normalizeAbsolutePath(path + fragment, hasPushState, root);
}

export function _resolveUrl(fragment, baseUrl, hasPushState) {
export function _resolveUrl(fragment, baseUrl, hasPushState, root) {
if (isRootedPath.test(fragment)) {
return _normalizeAbsolutePath(fragment, hasPushState);
return _normalizeAbsolutePath(fragment, hasPushState, root);
}

return _createRootedPath(fragment, baseUrl, hasPushState);
return _createRootedPath(fragment, baseUrl, hasPushState, root);
}

export function _ensureArrayWithSingleRoutePerConfig(config) {
Expand Down
40 changes: 40 additions & 0 deletions test/router.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,29 @@ describe('the router', () => {
});
});

it('should take root into account when creating push-state URIs', (done) => {
const child = router.createChild(new Container());
child.baseUrl = 'child-router';

Promise.all([
router.configure(config => config.map({ name: 'parent', route: 'parent', moduleId: './test' })),
child.configure(config => config.map({ name: 'child', route: 'child', moduleId: './test' }))
]).then(() => {
expect(router.generate('parent')).toBe('#/parent');
expect(child.generate('parent')).toBe('#/parent');
expect(child.generate('child')).toBe('#/child-router/child');

router.history.root = '/docs/';
router.history._hasPushState = true;

expect(router.generate('parent')).toBe('/docs/parent');
expect(child.generate('parent')).toBe('/docs/parent');
expect(child.generate('child')).toBe('/docs/child-router/child');

done();
});
});

it('should delegate to parent when not configured', (done) => {
const child = router.createChild(new Container());

Expand Down Expand Up @@ -140,6 +163,23 @@ describe('the router', () => {
done();
});
});

it('should return a fully-qualified URL when options.absolute is true and the root is set', (done) => {
const child = router.createChild(new Container());
let options = { absolute: true };
router.history.root = '/docs/';

child.configure(config => config.map({ name: 'test', route: 'test/:id', moduleId: './test' }))
.then(() => {
expect(child.generate('test', { id: 1 }, options)).toBe(`${absoluteRoot}#/test/1`);

router.history._hasPushState = true;

expect(child.generate('test', { id: 1 }, options)).toBe(`${absoluteRoot}test/1`);

done();
});
});
});

describe('navigate', () => {
Expand Down