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

Ionic 4 - StackController Page reuse uses old Page data #16516

Closed
KevinKelchen opened this issue Nov 29, 2018 · 26 comments
Closed

Ionic 4 - StackController Page reuse uses old Page data #16516

KevinKelchen opened this issue Nov 29, 2018 · 26 comments
Labels
package: angular @ionic/angular package type: bug a confirmed bug report

Comments

@KevinKelchen
Copy link

KevinKelchen commented Nov 29, 2018

Bug Report

Ionic Info

Ionic:

   ionic (Ionic CLI)             : 4.5.0 (/usr/local/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.0.0-beta.19
   @angular-devkit/build-angular : 0.10.7
   @angular-devkit/schematics    : 7.0.7
   @angular/cli                  : 7.0.7
   @ionic/angular-toolkit        : 1.2.0

System:

   NodeJS : v10.13.0 (/usr/local/bin/node)
   npm    : 6.4.1
   OS     : macOS Mojave

Describe the Bug
(There are arguably two interrelated but separate issues here. If I should create separate issues then please let me know! 🙂)

Forward navigation to a Page with the same URL (excluding query parameters) will modify the navigation stack history (not the browser history stack but the StackController stack, which are different). It will then re-use a previous Page instance that does not contain the latest navigation data from the forward navigation to the same URL.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Clone this repo on your machine: https://github.com/KevinKelchen/ionic4-stack-page-reuse . It is a slightly modified version of the sidemenu Angular stater template.
  2. In the terminal, path into the repo root and run npm i.
  3. Run ionic serve.
  4. Click the Menu icon and then List.
  5. Click Item 1 to forward navigate to the detail screen for Item 1.
    • You will notice there's a timestamp that displays. I chose a timestamp in this example so that there's an easy way to see on-screen what navigation data is being used.
    • This timestamp data was stored in a shared service prior to navigating from the source Page. It was then retrieved from the target Page from the same service. This pattern was outlined by Josh Morony here. The data in the service is keyed in a dictionary using the navId UUID URL query parameter.
    • A shared service is being used because we are migrating from Ionic 3 and the use of NavParams which allowed passing complex data like callback functions and objects which cannot be serialized in a lossless way in the URL or @ionic/storage.
    • A UUID was chosen so that navigating to what's otherwise the same URL will use the navigation parameters specific to the current navigation. This will also preserve previous navigation parameters so they can be used when revisiting a Page in the browser history.
  6. Click the pencil/create button in the header. This will navigate you to a different Page type called List Detail More.
  7. From this Page, click Item 1 to forward navigate to the detail screen for Item 1. Note that the timestamp is the old value that was displayed when you initially navigated to Item 1. This is because the StackController rolled back the stack and re-used the old instance of the Page for Item 1. The new navId URL query parameter was not used to retrieve the data from the shared service and thus is not using fresh data on this forward navigation.
  8. To observe the StackController navigation stack modification, click the ion-back-button and it will take you to the initial List Page instead of List Detail More which is the screen you were previously on before revisiting Item 1.

Related Code
The steps to reproduce above link to this repository which is set up to reproduce the issue.

Expected Behavior
I expected forward navigation to a Page with the same URL to use the latest, current navigation data from the forward navigation to the same URL and not modify the StackController navigation stack history.

Additional Context
Here is some additional context from a message I posted in #16367:


Note: My definition of "reusing" a Page is when "forward" navigating to a URL and instead of pushing a new Page onto the StackController's "stack" it re-uses an instance of the Page that is already present in the stack.

I've been having issues with StackController as well but on Beta 15. I am using the Angular router imperatively in TypeScript but I believe I was experiencing them as well using the new NavController (which uses the Angular router).

Here's a related behavior I've observed:

When you use the router to "forward" navigate to a Page, if the URL (excluding query parameters) is the same as a Page that is already in the "stack," the StackController will remove all of the previous Pages in the stack back to the first occurrence of the matching URL/Page. It then re-uses that first occurrence of the Page in the stack. And so then when you use ion-back-button, which seems to ignore browser history and only uses the stack, the Page you go back to is not what you would expect--it's the Page prior to the first occurrence of the Page in the stack.

  • Note: Since the Android and browser back button seems to ignore the StackController's "stack," I've tried working around this issue to some success by creating my own custom back button component that just uses Angular's Location.back() when you click it. This tends to result in a lot of destroyed/re-initializing of Pages whose state you can attempt to preserve by storing off data in a shared service that is keyed with a unique ID (such as a UUID) present in the URL.

How StackController re-uses Pages has actually been tripping me up in another way:

If I use the Angular router to "forward" navigate to a Page with the same URL as one in the StackController's stack and I have navigation parameters that I store in a shared service (as opposed to the URL) and key the parameter storage in the service off of a URL query parameter such as a UUID (a query parameter--not using the matrix URL notation as noted at the end of this section), because the Page is being re-used due to a matching URL instead of being considered as different, the new query parameter value is not being used and the Page pulls the previous parameters out of the shared service due to using the old query parameter value.

Perhaps the solve is to work around it by just putting the UUID in as a matrix URL notation parameter and effectively opt-out of any StackController reuse of Pages.

  • Upsides
    • "Forward" navigating to the same Page/URL would use the correct parameters.
    • With all of the Pages in the stack remaining wired up (never destroyed or reused), when you navigate back to them they are exactly the way you left them when you navigated away, which is how Ionic 3 worked.
  • Downsides
    • It's non-default behavior that differs from how Ionic seems to think it should work in Ionic 4.
    • Although it's like Ionic 3, you lose an optimization in which all Pages stay wired-up in the DOM as you continue to forward navigate through the app which may potentially increase the amount of memory that's used by the app; I would have concerns of how this might scale as users keep forward navigating through the app.

Thank you for all that you do and for making a great framework! 🙂

@KevinKelchen
Copy link
Author

Behavior is likewise the same with Beta 17.

Ionic Team: I can update the repo for reproducing the issue to Beta 17 if you wish. Please just let me know. 🙂

Thank you!

@bryce13950
Copy link

If you could update it for ionic beta 17 then that would be very helpful.

@KevinKelchen
Copy link
Author

@bryce13950, the repository for reproducing the issue has now been updated to Beta 17. Please let me know if there's anything else that you would like.

Thank you! 😃

@bryce13950
Copy link

I spent sometime playing around with this, and I think the issue stems from the reuse strategy in the app.module.ts. I simply removed the following...

    { provide: RouteReuseStrategy, useClass: IonicRouteStrategy }

Then the pages seemed to update properly. Maybe the ionic route strategy will just need to be removed for your project?

@bryce13950 bryce13950 added triage needs: reply the issue needs a response from the user and removed needs: investigation triage labels Dec 7, 2018
@bgpedersen
Copy link

I'm having the same page caching issue for several weeks now, which is starting to affect critical areas of my applictation for different situations. I am refreshing this thread everyday in hope a solution comes soon. I think it's a fine solution that pages pulled from cached if the user is pressing back/going back in history, but I don't think pages should ever be pulled from cache if navigating to a url (going forward) - even if the page has previously been visited.

Example 1:
User A is logged into the application and starts on the 'Home' page with info relevant to user A. User A logs out and navigates to 'Log In'.

User B logs in from the same application and navigates to 'Home' page. Ionic see's that Home was previously visited (by user A) and instead of instantiating 'Home', the view is pulled from cache and displays everything that User A was seing.

Example 2) A user clicks a recieved notification about a new post was created. This navigates to the post wall which will load the new post and show it in the wall. But if the user is already on the post wall or the post wall have been visited already, the component/page is pulled from cache and always display the old data without being able to hook into normal Angular behavior for ngOnInit.

--
Note:
I tried to remove the following as suggestedby @bryce13950 without any change:

{ provide: RouteReuseStrategy, useClass: IonicRouteStrategy }

I tried to use navigateRoot() as suggested by @manucorporat without success:
navController.navigateRoot()

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Dec 11, 2018
@bryce13950 bryce13950 removed their assignment Dec 11, 2018
@ionitron-bot ionitron-bot bot removed the triage label Dec 11, 2018
@bryce13950
Copy link

@Moelgaard85 Very strange. Could you provide a repo for this, so we have a couple of examples to look at?

@KevinKelchen Did removing that provider help you at all?

@KevinKelchen
Copy link
Author

@bryce13950, thank you for looking into this and for your response! Sorry it's taken several days to respond--I was sick last week and then spent time trying to investigate this issue further.

The suggestion to remove Ionic's implementation of RouteReuseStrategy does make the sample I provided appear to work as expected. I then tried it in our real application and it does not work, however. 😕 I'm still not certain why. It seems like the ActivatedRoute.queryParamMap Observable in the Page that's being re-used is not getting the updated value, but I'm not sure. I will continue looking into it. It does strike me as curious that we'd have to deviate from Ionic's default behavior with removing IonicRouteStrategy--I wonder what down the road may be trickier if we do that. At any rate, thus far it seems that the problem lies elsewhere.

@Moelgaard85, I'm glad to see that I'm not alone! I've been wondering if I'm just doing something unconventional in our application because no one else has commented on this issue experiencing the same thing. If anyone else is experiencing these challenges, please chime in! Hopefully we can then work together towards a solution. 😀

A thought:
As far as I know, Angular by default destroys and re-inits "Page" components between navigations such that only one "Page" component is in the DOM/wired-up/exists at a time. Ionic Beta 16 introduced this behavior which gave many users significant difficulties because they were depending on Pages in the navigation stack history to still exist. I was amongst these users. Beta 17 changed the behavior to work more like Beta 15 again.

Yesterday I pinged Mike Hartington on Slack asking,

In Ionic 3 the "stack" seemed to work such that every forward navigation added a Page to the DOM. With Ionic 4 and Angular routing it seems that the StackController on a "forward" navigation will try to pop Pages off the stack and re-use a Page instance earlier in the stack that has a matching URL. This has been an interesting change in our app coming from Ionic 3 as Pages get destroyed/re-initialized and even re-used. Do you know what the rationale is for that new behavior? Trying to be more memory efficient? Something else?

He graciously responded with,

mmm, more that we're working with the angular router and it's ideas of how things should work

I did not ask for more details. Perhaps the thought was to find a middle-ground between Angular and Ionic 3 where there are still Pages in the DOM (the "stack") but they can eventually still be destroyed?

What if it could work such that when you use the Angular router to "forward" navigate to a Page that it will always add a new Page instead of rolling back the StackController stack and re-using an existing Page with matching URL? At the same time, perhaps it could actually remove the existing instances of the same Page with matching URL from the stack and still have some of that Page clean-up take place? If you then navigate back through history it would then re-create the destroyed Page when it needed to (similar to when using Angular's Location.back() or the browser/Android back button to go back to a Page where the Page was deleted from the StackController stack so it re-inits it)? Might have to think about this more.

Thank you!

@KevinKelchen
Copy link
Author

@bryce13950, I've updated the sample project which reproduces the issue as well as the steps to reproduce in the original post. I left the providing of IonicRouteStrategy in place since that's the default; you will once again need to remove it in order to test using Angular's default RouteReuseStrategy.

After debugging through Angular and Ionic navigation code I was able to understand the code path differences between the sample project and our actual application. Turns out I needed to add navigating to a different Page before forward navigating to the same Page/URL again (like our actual application).

@bgpedersen
Copy link

bgpedersen commented Dec 13, 2018

This new article on Medium about ionic 4 and the lifecycle hooks might address our some of the issues we are experiencing: article

I am unsure if this article helps us, so I would recommend not closing this github issue until we are certain a bug exists or we need to use the extended Ionic router functionality as explained in the article instead.

@KevinKelchen
Copy link
Author

KevinKelchen commented Dec 13, 2018

Thanks, @Moelgaard85. That’s a good article from Paul on clearing up misconceptions regarding the existence of Ionic lifecycle hooks in v4.

Using something like ionViewWillEnter instead of ngOnInit doesn't fix my issue with outdated routing data or stack history modification, however.

The post also refers to the vanilla Angular router outlet. This router outlet sounds like it would function more like Beta 16 in which there is no stack at all and only one Page exists in the DOM at a time. I imagine you would also lose the routing animations Ionic provides and perhaps other things. Like Beta 16, I’m not sure these characteristics would be the best fit for us as, like others who had issues with Beta 16, we depend on prior Pages still existing in the DOM. Even if Angular’s outlet seemed to otherwise “work” it would probably be an unconventional configuration that may not be well supported or tested.

@mhartington
Copy link
Contributor

So there's a lot here right now, but if you're looking to not create new instances of pages in V4, just disable the ionic route reuse strategy.

This was added as a way to maintain the same behavior as V3.

@KevinKelchen
Copy link
Author

KevinKelchen commented Dec 17, 2018

Hi @mhartington! 👋

My hope kind of is to create new instances of Pages when forward navigating like v3 did--not to use cached Pages like v4 has introduced. Like @bryce13950 also suggested, I did indeed remove { provide: RouteReuseStrategy, useClass: IonicRouteStrategy } in hopes that cached Pages would not be used, but that only helped when navigating directly from one Page to another Page of the same Page type; navigating to a different Page type in between still results in a cached Page in the stack being used instead of instantiating a new Page. I've also tried creating a custom RouteReuseStrategy where shouldReuseRoute() simply returns false. No dice--cached Pages are still being re-used when I would have expected a new Page to be created. Again, the steps to reproduce will demonstrate the behavior; I just updated the referenced sample project to Beta 19 as well.

For me it's almost like: I want to have a stack of Pages still in the DOM so when I back-navigate the Page instance is still there (at least the prior Page). However, when I forward navigate I want a fresh Page that's not re-using stuff. At the same time, it's probably not great to keep adding Page after Page to the DOM from a resource consumption standpoint like v3 did. Perhaps there's a middle ground where Pages further back in the history can be cleaned up as you use the application, such as when you forward navigate to the same URL it doesn't pop the stack back to, and re-use, the prior instance of the Page, but rather silently deletes the prior instance of the Page from the DOM. Then when you go back in history to that Page it will be re-initialized. Or something like that.

Perhaps this new behavior of using a cached Page in the StackController stack could work for me, but one of the main blockers to that is this very issue which has also been reported more specifically as #16736. Per that issue, if the routing observables fire appropriately we may be able to get by with using cached Pages.

Thank you for all of your time and assistance! 😀

@Jahrenski
Copy link

The same happens to my project as well. I can't seem to find a single way to make it work properly.
It's either the login and home pages that are re-used or all of the other pages.

I deleted the Ionic reuse strategy to no avail.

All my in page links using the routerDirection="root" will navigate and destroy any pages but the new one. But pages navigated using the imperative Router.navigate() or navigateByUrl() will always stay in memory. replaceUrl: true will not fix this.

@softsimon
Copy link

softsimon commented Feb 11, 2019

My solution to this issue was using a event emitter Obvservable Subjects to notify the cached view to reload the data.

In a service:

private dataUpdatedEvent: Subject<any> = new Subject();

 emitDataUpdatedEvent() {
   this.dataUpdatedEvent.next();
 }

 getDataUpdatedEvent$(): Observable<boolean> {
   return this.dataUpdatedEvent.asObservable();
 }

In the view that has cache problems:

  dataUpdatedEvent: Subscription;

  ngOnInit() {
    this.dataUpdatedEvent = this.myService.getDataUpdatedEvent$()
      .subscribe(() => {
        this.loadData();
      });
  }

  ngOnDestroy(): void {
    this.dataUpdatedEvent.unsubscribe();
  }

  ionViewWillEnter() {
    this.loadData();
  }

If the view is cached when I call emitDataUpdatedEvent() the data will be reloaded in the view.

@zdennis
Copy link

zdennis commented Feb 21, 2019

Thinking that this may be fixed at some point in the future I took an approach to re-use the resolvers on existing route configs to load data for the ActivatedRoute:

https://gist.github.com/zdennis/2f73f60b4bfbab6b98ca3d2b9bfbc1fd#file-route-data-resolution-service-ts

I've attached a basic component to show how I'm using it.

This will call your resolver twice when navigating back to a cached page (once for the new activated route that Angular has made that you don't have access to and once for your cached activated route that the component has reference to). This is not a concern for me as we'll caching in the service layer during navigation transitions can be used to avoid calling the server twice.

@daem0ndev
Copy link
Contributor

@KevinKelchen could you try the latest nightly and see if you're still having issues here?

@liamdebeasi
Copy link
Contributor

Hi @KevinKelchen,

I tried out the repository you provided, and it looks like this issue has been resolved by #17888. Would you be able to try it out using the latest Ionic dev build to ensure that everything is working as you would expect? You can install the nightly using npm i @ionic/core@dev @ionic/angular@dev.

Thanks!

@liamdebeasi liamdebeasi added package: angular @ionic/angular package type: bug a confirmed bug report labels Mar 29, 2019
@KevinKelchen
Copy link
Author

@daem0ndev and @liamdebeasi,

I will give it a shot!

Thank you! 😀

@KevinKelchen
Copy link
Author

I am experiencing the same behavior as before.

Installing the nightly changed my package.json to use the following versions:

"@ionic/angular": "^4.2.0-dev.201903291804.8ba82fc",
"@ionic/core": "^4.2.0-dev.201903291804.8ba82fc",

@liamdebeasi, is running that command to install the nightly and following the steps to reproduce what you did to check if it resolved the issue? Did you make any other changes?

Thanks!

@KevinKelchen
Copy link
Author

@daem0ndev reached out to me in effort to reproduce the issue. 😀

I'm not sure why it wasn't appearing during my initial attempts, but after blowing away node_modules and trying again I began seeing TypeError: this.router.getCurrentNavigation is not a function in the console. It looks like that method was introduced in Angular 7.2.0-beta.1. My sample app repo's lockfile was using an older version of Angular 7.

After updating to Angular 7.2.x, it works as expected! 🎉 Thank you, @daem0ndev!

@KevinKelchen
Copy link
Author

As I've learned more, I think there are two related "issues" in this single GitHub issue:

  1. ActivatedRoute Observables are not firing when using a cached Page.
    • This seems to be fixed now in the nightly version of Ionic thanks to @daem0ndev.
  2. Coming from Ionic 3 and not using ion-tabs I was expecting a linear navigation stack.
    • When forward navigating to a cached Page, preceding Pages in the Ionic stack are popped off/destroyed. Using ion-back-button to go back doesn't take you to the screen you were previously looking at but rather the screen that's before the cached Page instance. Using a custom back button that uses Angular's Location.back() or the browser back button uses browser history and thus works differently and works as I would have expected. Downsides though include wondering if I'm going against Ionic's intended non-linear navigation design as well as Page instances having to be re-initialized (not a huge deal; works closer to vanilla Angular). Perhaps a separate feature request "issue" could be used for that discussion.

I'm going to close this issue since the primary issue is resolved. 🙂

Thank you!

@Topiya
Copy link

Topiya commented Apr 24, 2019

@KevinKelchen I have tried npm i @ionic/core@dev @ionic/angular@dev and my package.json changed as below:

    "@ionic/angular": "^4.3.0-dev.201904171418.07e739a",
    "@ionic/core": "^4.3.0-dev.201904171418.07e739a",

and the Angular version is "@angular/common": "^7.2.2",

But still I am facing the problem like, when I navigate back to last page of stack, page reuses the old data.

Can you please suggest me on this?

@karvannan-thi
Copy link

my package.json
"@ionic/angular": "4.1.0",

me too facing the same problem

@KevinKelchen
Copy link
Author

Hi @Topiya and @karvannan-thi, 👋

This was fixed in Ionic Angular 4.2.0.
https://github.com/ionic-team/ionic/releases/tag/v4.2.0

@Topiya,
Are all of your Angular packages upgraded? Not just @angular/common?

@sunrisetechdoc
Copy link

sunrisetechdoc commented May 11, 2019

My package.json
"@angular/common": "^7.2.2",
"@angular/core": "^7.2.2",
"@angular/forms": "^7.2.2",
"@angular/http": "^7.2.2",
"@angular/router": "^7.2.2",
"@ionic-native/core": "^5.0.0",
"@ionic/angular": "^4.2.0",
"cordova-android": "8.0.0",

me too facing the same problem

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 10, 2019

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: angular @ionic/angular package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests