search

aurelia / router

A powerful client-side router.
MIT License
120 stars 115 forks source link

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

Open jwx opened 5 years ago

jwx commented 5 years ago

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/router#528.

jwx commented 5 years ago

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

davismj commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@jwx Any progress on the tests?

jwx commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

davismj commented 5 years ago

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

gama410 commented 5 years ago

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 commented 5 years ago

@davismj Can you comment on this?

davismj commented 5 years ago

@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 https://github.com/aurelia/router/pull/621#issuecomment-441248796).

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 commented 5 years ago

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

avrahamcool commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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).