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.

Sign up for GitHub

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): canDeactivate correctly cancels browser history navigation #621

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(router): canDeactivate correctly cancels browser history navigation #621

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e82646
fix(router): canDeactivate correctly cancels browser history navigation
jwx Oct 8, 2018
3a66b74
Configured debugging for Karma and VS Code
jwx Oct 8, 2018
f4343e4
Replaced history.history with history
jwx Oct 9, 2018
a525f63
Contains test code that is NOT WORKING
jwx Nov 13, 2018
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
16 changes: 14 additions & 2 deletions src/app-router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,20 @@ export class AppRouter extends Router {
this.isNavigatingForward = true;
} else if (this.currentNavigationTracker > navtracker) {
this.isNavigatingBack = true;
} if (!navtracker) {
}
if (!navtracker) {
navtracker = Date.now();
this.history.setState('NavigationTracker', navtracker);
}
this.currentNavigationTracker = navtracker;

let historyIndex = this.history.getHistoryIndex();
this.lastHistoryMovement = historyIndex - this.currentHistoryIndex;
if (isNaN(this.lastHistoryMovement)) {
this.lastHistoryMovement = 0;
}
this.currentHistoryIndex = historyIndex;

instruction.previousInstruction = this.currentInstruction;

if (!instructionCount) {
Expand Down Expand Up @@ -256,7 +264,11 @@ function resolveInstruction(instruction, result, isInnerInstruction, router) {
function restorePreviousLocation(router) {
let previousLocation = router.history.previousLocation;
if (previousLocation) {
router.navigate(router.history.previousLocation, { trigger: false, replace: true });
Promise.resolve().then(() => {
davismj marked this conversation as resolved.
Show resolved Hide resolved
if (router.lastHistoryMovement && !isNaN(router.lastHistoryMovement)) {
router.history.history.go(-router.lastHistoryMovement);
Copy link
Member

@bigopon bigopon Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does this read "assume the History implementation from Aurelia always have a history pointing to underlying platform History"? @jwx

Copy link
Member Author

Choose a reason for hiding this comment

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

Err, yeah, unfortunately. It should be rewritten so that it only goes to the History implementation (which already requires navigateBack so go or move should be acceptable there). Once @davismj has taken a look at this "preliminary" PR and approved of my solution to the problem, I'll fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I like your PRs to enhance the aurelia's History implementation. Looking towards it.

}
});
} else if (router.fallbackRoute) {
router.navigate(router.fallbackRoute, { trigger: true, replace: true });
} else {
Expand Down
10 changes: 10 additions & 0 deletions src/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ export class Router {
*/
currentNavigationTracker: number;

/**
* The current index in the browser history.
*/
currentHistoryIndex: number;

/**
* The amount of index steps in the last history navigation
*/
lastHistoryMovement: number;

/**
* The navigation models for routes that specified [[RouteConfig.nav]].
*/
Expand Down
64 changes: 55 additions & 9 deletions test/app-router.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,48 @@ import {RouteLoader} from '../src/route-loading';
import {Pipeline} from '../src/pipeline';

class MockHistory extends History {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sold on this implementation for tests. In my experience, when you build a mock object that does more than noops, it opens the door wide open for bad integration tests, when the mock doesn't match the class you're mocking. I'd love to have @fkleuver weigh in on this approach.

Copy link
Member

Choose a reason for hiding this comment

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

#621 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

A mock isn't necessarily meant to be used for integration tests in the first place. In an integration test you use real implementations. A mock allows you to unit test a specific piece of behavior by making the mock return some pre-configured values and to verify that the SUT calls the component in the correct manner.
Here's an example of a pass-through expression (that technically speaking is a spy, not a mock) that I use to fully verify the interactions. It's used by integration tests: https://github.com/aurelia/aurelia/blob/master/packages/runtime/test/unit/mock.ts#L853
Here's an example of something that's more like a mock, where I short circuit about 5 different components by passing some hard-coded values in the constructor, and it still mostly mimics the real behavior: https://github.com/aurelia/aurelia/blob/master/packages/runtime/test/unit/mock.ts#L377
Neither of these by themselves would ever be sufficient for testing a particular piece, but they are useful for isolating things on a more granular level. There still need to be full integration tests in addition.

So if this mock serves the purpose of granular verification, that's fine, just by itself it isn't enough.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking in this case we want to mock the browser's history object, but want to use the actual implementation of history-browser for the tests. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

As in any other case I'd say preferably both. But it might not be possible to use the actual browser's history object without, well, navigating away from the testing page and crashing the tests :) I haven't tried that before, I wonder if it's possible

history;
activate() {}
deactivate() {}
navigate() {}
navigate(location) {
this.history.navigate(location)
}
navigateBack() {}
setState(key, value) {}
setState(key, value) {
this.history.setState(key, value);
}
getState(key) {
return this.history.getState(key);
}
}

class MockBrowserHistory {
currentLocationIndex = 0;
states = [];
get length() {
return this.states.length;
}
setState(key, value) {
if (!this.states[this.currentLocationIndex]) {
this.states[this.currentLocationIndex] = {};
}
this.states[this.currentLocationIndex][key] = value;
}
getState(key) {
return null;
if (!this.states[this.currentLocationIndex]) {
this.states[this.currentLocationIndex] = {};
}
return this.states[this.currentLocationIndex][key];
}
location(location) {
this.states.splice(this.currentLocationIndex + 1);
this.currentLocationIndex = this.states.length;
this.states.push({ HistoryIndex: this.currentLocationIndex });
return location;
}
go(movement) {
this.currentLocationIndex += movement;
console.log('GO', this.currentLocationIndex, this.states);
}
}

Expand Down Expand Up @@ -42,6 +77,7 @@ describe('app-router', () => {

beforeEach(() => {
history = new MockHistory();
history.history = new MockBrowserHistory();
container = new Container();
container.registerSingleton(RouteLoader, MockLoader);
ea = { publish() {} };
Expand Down Expand Up @@ -208,12 +244,17 @@ describe('app-router', () => {
describe('loadUrl', () => {
it('restores previous location when route not found', (done) => {
spyOn(history, 'navigate');
spyOn(history.history, 'go');

router.history.previousLocation = 'prev';
router.loadUrl('next')
router.history.previousLocation = router.history.history.location('prev');

router.lastHistoryMovement = 1;
router.loadUrl(router.history.history.location('next'))
.then(result => {
expect(result).toBeFalsy();
expect(history.navigate).toHaveBeenCalledWith('#/prev', { trigger: false, replace: true });
// Navigation is now restored through the browser history
// expect(history.navigate).toHaveBeenCalledWith('#/prev', { trigger: false, replace: true });
expect(history.history.go).toHaveBeenCalledWith(-1);
})
.catch(result => expect(true).toBeFalsy('should have succeeded'))
.then(done);
Expand All @@ -235,19 +276,24 @@ describe('app-router', () => {

it('restores previous location on error', (done) => {
spyOn(history, 'navigate');
spyOn(history.history, 'go');

router.history.previousLocation = router.history.history.location('prev');

router.history.previousLocation = 'prev';
router.activate();
router.configure(config => {
config.map([
{ name: 'test', route: '', moduleId: './test' }
]);
});

router.loadUrl('next')
router.lastHistoryMovement = 1;
router.loadUrl(router.history.history.location('next'))
.then(result => {
expect(result).toBeFalsy();
expect(history.navigate).toHaveBeenCalledWith('#/prev', { trigger: false, replace: true });
// Navigation is now restored through the browser history
// expect(history.navigate).toHaveBeenCalledWith('#/prev', { trigger: false, replace: true });
expect(router.history.history.go).toHaveBeenCalledWith(-1);
})
.catch(result => expect(true).toBeFalsy('should have succeeded'))
.then(done);
Expand Down