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

jwx · 2018-10-08T14:13:31Z

Preventing a navigation with canDeactivate doesn't cancel the browser history navigation. This fix moves the browser back to the index where navigation was cancelled.

Depending on PR aurelia/history-browser#42.
Closes #528.

Preventing a navigation with canDeactivate doesn't cancel the browser history navigation. This fix moves the browser back to the index where navigation was cancelled. Depending on PR aurelia/history-browser#42. Closes aurelia#528.

jwx · 2018-10-08T14:15:22Z

Due to the lack of tests performed (pushstate, other browsers than Chrome, etc) this is a somewhat preliminary PR.

bigopon · 2018-10-08T23:09:52Z

src/app-router.js

-    router.navigate(router.history.previousLocation, { trigger: false, replace: true });
+    Promise.resolve().then(() => {
+      if (router.lastHistoryMovement && !isNaN(router.lastHistoryMovement)) {
+        router.history.history.go(-router.lastHistoryMovement);


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

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.

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

src/app-router.js

davismj · 2018-10-09T05:11:48Z

Overall, I think this is great and a workable strategy. Wish I had thought of using the history state API. Nice work @jwx.

As @bigopon mentioned, let's go ahead and implement an abstraction of history.go on the history and history-browser modules and use that instead.

There's one remaining edge case bug that I'm not sure we're going to solve here. Let's say your history looks like this:

One <== current
Two
One
Three

If you use history.go(-2) or use the browser history buttons to navigate back two pages, hash change doesn't trigger, so the code here isn't called, nor is a canDeactivate() callback, and the navigation succeeds from the browser's perspective, and you end up here:

One
Two
One <== current
Three

Assume canDeactivate in One should have failed. When the user navigates back, the developer might expect the user to end up at Two, however the user ends up at Three.

I believe this bug is out of the scope of the original bug report and I'd be fine letting it ride for now. It won't break anyone's application.

jwx · 2018-10-09T09:22:53Z

@davismj Yeah, I've noticed that edge case as well. I agree with you on scope and would rather say that the edge case is due to a navigation bug where (some of) the life cycle isn't invoked.

I'll implement the history abstractions and make the router changes later today or tomorrow.

davismj · 2018-10-10T03:01:05Z

@jwx

Seems you added some extra vscode and karma edits by accident that need to be removed.

Core code is perfect. Good work.

I'm not sold on the tests, and I really want to make sure the tests are solid because we've definitely burned a few Aurelia developers in the past on issues that could have been solved through better testing. I've pinged @fkleuver for his expert opinion.

fkleuver · 2018-10-10T04:31:52Z

Nice work!

I like the mock, it helps verify the low-level interactions of these changes. I would personally add a small loop (or a few copy-pastes) to verify different numeric values for just that extra bit of confidence. I'm only spotting -1.

In addition to the mock, it's probably also a good idea to have an integration test with the actual BrowserHistory implementation and verify the final outcome as well.

I made the horrible mistake of installing node 10 on my windows box and of course this gulp project breaks. I can't verify the test coverage. Did you check that all the changes have coverage? gulp cover should generate the report

davismj · 2018-10-10T03:01:22Z

.vscode/launch.json

@@ -0,0 +1,30 @@
+{


Shouldn't be here.

davismj · 2018-10-10T03:01:32Z

karma.conf.js

@@ -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'],


Shouldn't be here

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

I'm not saying it's not a good change, I'm just saying its out of the scope of this particular PR.

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

davismj · 2018-10-10T03:04:09Z

test/app-router.spec.js

@@ -5,13 +5,48 @@ import {RouteLoader} from '../src/route-loading';
 import {Pipeline} from '../src/pipeline';

 class MockHistory extends History {


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.

#621 (comment)

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.

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?

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

davismj · 2018-11-07T05:31:28Z

@jwx Any progress on the tests?

jwx · 2018-11-07T16:39:59Z

Yeah, one test left to go. Unfortunately, it's a bit of a struggle. Hopefully I'll be able to sort it out tonight or tomorrow.

davismj · 2018-11-23T14:03:40Z

@jwx I've started working on fixing the tests. Please see my commits here: https://github.com/davismj/router/tree/fix/can-deactivate-history. You're going to want to rebase onto this, or create a new branch and pull all these commits, as I've rebased on the TypeScript refactor for you.

After struggling with the first test, which restores the previous location on not-found, I found the issue wasn't the tests but the feature. lastHistoryMovement is set in _dequeueInstruction, which is never called in the case of a not found route, where the instruction is never created (see https://github.com/aurelia/router/blob/master/src/router.ts#L614). Take a look at fixing this and I will fix the rest of the tests.

doktordirk · 2019-02-08T14:37:08Z

@davismj @jwx still movement here? i wouldn't mind using the fix 🤗

davismj · 2019-02-19T10:05:17Z

@jwx have you had any time to look at this and get it fixed?

gama410 · 2019-05-22T14:36:58Z

Hi guys! I'm coming back to check how this fix is going... I see all tests have passed, can't we just fix the conflicts and merge this so we could have it in next month release?

EisenbergEffect · 2019-05-27T01:18:05Z

@davismj Can you comment on this?

davismj · 2019-05-28T14:47:32Z

@gama410 So the history here is I tried to fix it and couldn't. @jwx tried to fix it and made better progress but there were still issues. I tried to fix the issues but again couldn't (see #621 (comment)).

There has been no activity since then. @bigopon has expressed an interest in fixing it.

This is a non-trivial bug that will take a significant effort to repair. I've already spent days looking into it with little success. I apologize for the inconvenience it has caused, but please understand that we are not simply ignoring you.

gama410 · 2019-05-28T15:13:22Z

@davismj I understand totally, don't worry. I'm glad to have some feedback. I hope you'll get to a working solution!

avrahamcool · 2023-09-19T18:45:52Z

Hi everyone,

This PR has been stalled for quite some time now.
I understand from the comments that some cases are not addressed by this PR, and it's a difficult problem to resolve.
Is there a reason why we can't merge this PR? Fixing most of the simple cases is a better option than nothing.

Due to the same bug in our production app, our team is becoming interested in this fix.
I would like to assist here as well, but I was unclear about the specifics of those difficult situations.

bigopon · 2023-09-20T01:40:39Z

Thanks @avrahamcool . One of the reasons why this wasn't moved quickly was because it wasn't sufficiently tested. We need better testing setup, with probably real browser history and interaction. We can use either cypress/playwright for this. I'll gladly support any PR as long as we have that test setup. I can also help set it up if necessary.

avrahamcool · 2023-09-20T18:59:38Z

Hi @bigopon,
I tried to replicate the suggested solution to our app - just to observe how it would function locally.
In cases involving 'canDeactivate' and 'activationStrategy.replace', the router appears to break.
It's possible that it breaks on simpler cases as well.
I don't have any test cases ready to demonstrate that yet - but I'm looking forward to having time this weekend to demonstrate it.
Your suggestion on setting up the test environment would be greatly appreciated. (I'm not confident in my abilities to do that).

Configured debugging for Karma and VS Code

3a66b74

jwx mentioned this pull request Oct 8, 2018

feat(history) add history index getter aurelia/history-browser#42

Merged

bigopon reviewed Oct 8, 2018

View reviewed changes

davismj reviewed Oct 9, 2018

View reviewed changes

src/app-router.js Show resolved Hide resolved

jwx mentioned this pull request Oct 9, 2018

feat(history) add go method aurelia/history-browser#43

Merged

Replaced history.history with history

f4343e4

davismj requested changes Oct 10, 2018

View reviewed changes

Contains test code that is NOT WORKING

a525f63

janat08 mentioned this pull request Dec 6, 2018

canDeactivate() only fired once when using navigation buttons (back/forward) aurelia/framework#731

Open

davismj mentioned this pull request Feb 19, 2019

Cancel navigateBack from canDeactivate #528

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jwx commented Oct 8, 2018

jwx commented Oct 8, 2018

bigopon Oct 8, 2018 •

edited

Loading

jwx Oct 8, 2018

bigopon Oct 8, 2018

davismj commented Oct 9, 2018 •

edited

Loading

jwx commented Oct 9, 2018

davismj commented Oct 10, 2018 •

edited

Loading

fkleuver commented Oct 10, 2018

davismj Oct 10, 2018

davismj Oct 10, 2018

fkleuver Oct 10, 2018

davismj Oct 10, 2018

fkleuver Oct 10, 2018

davismj Oct 10, 2018

davismj Oct 10, 2018

fkleuver Oct 10, 2018

davismj Oct 11, 2018

fkleuver Oct 11, 2018

davismj commented Nov 7, 2018

jwx commented Nov 7, 2018

davismj commented Nov 23, 2018 •

edited

Loading

doktordirk commented Feb 8, 2019

davismj commented Feb 19, 2019

gama410 commented May 22, 2019

EisenbergEffect commented May 27, 2019

davismj commented May 28, 2019 •

edited

Loading

gama410 commented May 28, 2019

avrahamcool commented Sep 19, 2023

bigopon commented Sep 20, 2023

avrahamcool commented Sep 20, 2023

		@@ -5,13 +5,48 @@ import {RouteLoader} from '../src/route-loading';
		import {Pipeline} from '../src/pipeline';

		class MockHistory extends History {

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

Are you sure you want to change the base?

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

Conversation

jwx commented Oct 8, 2018

jwx commented Oct 8, 2018

bigopon Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davismj commented Oct 9, 2018 • edited Loading

jwx commented Oct 9, 2018

davismj commented Oct 10, 2018 • edited Loading

fkleuver commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davismj commented Nov 7, 2018

jwx commented Nov 7, 2018

davismj commented Nov 23, 2018 • edited Loading

doktordirk commented Feb 8, 2019

davismj commented Feb 19, 2019

gama410 commented May 22, 2019

EisenbergEffect commented May 27, 2019

davismj commented May 28, 2019 • edited Loading

gama410 commented May 28, 2019

avrahamcool commented Sep 19, 2023

bigopon commented Sep 20, 2023

avrahamcool commented Sep 20, 2023

bigopon Oct 8, 2018 •

edited

Loading

davismj commented Oct 9, 2018 •

edited

Loading

davismj commented Oct 10, 2018 •

edited

Loading

davismj commented Nov 23, 2018 •

edited

Loading

davismj commented May 28, 2019 •

edited

Loading