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 all commits
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
30 changes: 30 additions & 0 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be here.

// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"type": "chrome",
"request": "launch",
"name": "Karma Tests",
"sourceMaps": true,
"webRoot": "${workspaceRoot}",
"urlFilter": "http://localhost:9876/debug.html",
// "runtimeArgs": [
// "--headless"
// ],
"pathMapping": {
"/": "${workspaceRoot}",
"/base/": "${workspaceRoot}/"
},
"sourceMapPathOverrides": {
"webpack:///./*": "${webRoot}/*",
"webpack:///src/*": "${webRoot}/*",
"webpack:///*": "*",
"webpack:///./~/*": "${webRoot}/node_modules/*",
"meteor://💻app/*": "${webRoot}/*"
}
}
]
}
9 changes: 7 additions & 2 deletions karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,14 @@ module.exports = function(config) {

// start these browsers
// available browser launchers: https://npmjs.org/browse/keyword/karma-launcher
browsers: ['Chrome'],

browsers: ['ChromeDebugging'],
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be here

Copy link
Member

Choose a reason for hiding this comment

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

Why not? This allows you to attach vs code to chrome and debug from within vs code

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 saying it's not a good change, I'm just saying its out of the scope of this particular PR.

Copy link
Member

Choose a reason for hiding this comment

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

hmmm I guess I can't really argue with that.. I must admit we've gotten a bit lax with scope creep on the vNext repo for the sake of progressing faster, but probably good to keep it a bit more strict here


customLaunchers: {
ChromeDebugging: {
base: 'Chrome',
flags: ['--remote-debugging-port=9333']
}
},
// Continuous Integration mode
// if true, Karma captures browsers, runs the tests and exits
singleRun: false
Expand Down
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.go(-router.lastHistoryMovement);
}
});
} 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
123 changes: 112 additions & 11 deletions test/app-router.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,74 @@
import {History} from 'aurelia-history';
import {History,BrowserHistory,LinkHandler} from 'aurelia-history';
import {Container} from 'aurelia-dependency-injection';
import {AppRouter} from '../src/app-router';
import {RouteLoader} from '../src/route-loading';
import {Pipeline} from '../src/pipeline';
import {PipelineProvider} from '../src/pipeline-provider';
// import 'aurelia-polyfills';

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 null;
return this.history.getState(key);
}
getHistoryIndex() {
let historyIndex = this.getState('HistoryIndex');
if (historyIndex === undefined) {
historyIndex = this.history.length - 1;
this.setState('HistoryIndex', historyIndex);
}
return historyIndex;
};
go(movement) {
return this.history.go(movement);
}
}

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) {
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);
}
}

class MockLoader extends RouteLoader {
loadRoute(router, config) {
return Promise.resolve({
viewModel: {}
viewModel: {
canDeactivate: () => { console.log('canDeactivate'); return false; }
}
});
}
}
Expand All @@ -42,6 +92,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 +259,17 @@ describe('app-router', () => {
describe('loadUrl', () => {
it('restores previous location when route not found', (done) => {
spyOn(history, 'navigate');
spyOn(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.go).toHaveBeenCalledWith(-1);
})
.catch(result => expect(true).toBeFalsy('should have succeeded'))
.then(done);
Expand All @@ -235,23 +291,68 @@ describe('app-router', () => {

it('restores previous location on error', (done) => {
spyOn(history, 'navigate');
spyOn(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.go).toHaveBeenCalledWith(-1);
})
.catch(result => expect(true).toBeFalsy('should have succeeded'))
.then(done);
});

// NOT WORKING
it('restores previous location after history navigation', (done) => {
spyOn(history, 'navigate');
spyOn(history, 'go');

const provider = new PipelineProvider(container);
const router = new AppRouter(container, history, provider, ea);

router.activate();
router.configure(config => {
config.map([
{ name: 'first', route: 'first', moduleId: './first' },
{ name: 'second', route: 'second', moduleId: './second' },
{ name: 'third', route: 'third', moduleId: './third' },
]);
}).then(() => {
router.loadUrl(router.history.history.location('first'))
.then(() => router.loadUrl(router.history.history.location('second')))
.then(() => router.loadUrl(router.history.history.location('third')))
.then(result => {
expect(result).toBeTruthy();
})
.catch(result => expect(true).toBeFalsy('should have succeeded'))
.then(done);
});


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

});
describe('instruction completes as navigation command', () => {
it('should complete instructions in order before terminating', done => {
Expand Down