Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

The suggested "guard" approach with ionViewCanEnter/ionViewCanLeave appears to lack configuration, leak objects, and could hide dangerous code. #281

Open
ionitron-bot bot opened this issue Nov 28, 2018 · 0 comments
Labels

Comments

@ionitron-bot
Copy link

ionitron-bot bot commented Nov 28, 2018

Original issue by @Barryrowe on 2017-05-01T13:39:38Z

Ionic version: (check one with "x")
[ ] 1.x
[x] 2.x
[x] 3.x

I'm submitting a ... (check one with "x")
[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior:

When implementing the suggested "guard" approach for Ionic, using the ionViewCanEnter hook on a page, there are a handful of issues that can arise with its implementation:

Lack Configuration
There is currently no configurable way to tell the navController where to go by default if an ionViewCanEnter returns/resolves false as the first route (a very possible case with DeepLinking). All you can do at the moment is inject navCtrl and manually navigate the user. This is tedious, and error prone when you have many pages that could receive a DeepLink that needs a guard. Being able to set a default redirect for a rejected ionCanViewEnter makes sense. Or maybe ionViewCanEnter needs to return an object like: { canEnter: boolean, redirectPage?: any }, so they can still override it. This is related to #11405

Leak Objects
If the ionViewCanEnter function returns/resolves false the newly instantiated component is never destroyed. (ngOnDestroy is never called).

Hide Dangerous Code
Unlike the core angular router guards, this implementation requires an instance of the component to be instantiated. This means that a developer could write some logic in the constructor for the component that they don't expect to run, that will always run regardless of the result of ionViewCanEnter

This paired with the lack of guaranteeing the components are destroyed, could lead to memory leaks, additional subscriptions, etc. Ex: it's not uncommon for Observable subscriptions to be created in a constructor, and if they aren't unsubscribed, this can prevent the component from being GC'd, as well as potentially duplicating the observable chain logic.

LazyLoading
In addition to the above items, if you're setting up lazy loaded modules with DeepLinking, the way guarding is handles means a whole module bundle has to be requested, loaded, and run before the decision is made that "Hey you shouldn't even be here"

Expected behavior:
I know there are complications with this, but the way the core angular router handles guarding seems to be the correct approach. Guarding at the navigation level and not at the page component level. This is where you have the chance to prevent the component from even being built, or loading a module that hasn't been yet loaded in lazy loading scenarios.

Steps to reproduce:

A simple example of not getting destroyed and not redirecting. Doesn't show the mulitiples as I didn't implement tabs.
http://plnkr.co/edit/Y7LNSD8xy2q6TcPPpXQf?p=preview

To create a more complete example

  1. Create a project with the tabs template

  2. Implement ionCanViewenter in the home page component to return false

  3. Implement ngOnDestroy in the home page to console log when it is destroyed

  4. Add a console.log line to the constructor for the home page to show it's being instantiated.

  5. Run the app, open a console, and see that A) the page doesn't default to loading anything since the tabs page can't load it's view and B) ngOnDestroy is never called on the home page even though it is constructed.

Related code:

ionViewCanEnter(){
 //You can uncomment this to 'redirect' to the About page and see that multiple attempts to navigate to Home will result in multiple constructions and no destroys.
 //this.navCtrl.setRoot(AboutPage);
 return false;
}

ngOnDestroy(){
   console.log("sup nerds? I should be destroyed now right?");
}

Other information:
Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

Cordova CLI: 6.5.0
Ionic Framework Version: 3.0.1
Ionic CLI Version: 2.2.2
Ionic App Lib Version: 2.2.1
Ionic App Scripts Version: 1.3.0
ios-deploy version: Not installed
ios-sim version: Not installed
OS: Windows 7
Node Version: v7.9.0
Xcode version: Not installed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

0 participants