-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
The suggested "guard" approach with ionViewCanEnter/ionViewCanLeave appears to lack configuration, leak objects, and could hide dangerous code. #11459
Comments
Hello @Barryrowe , thanks for the detailed issue! We will look into this. |
Very good post @Barryrowe and I'm 100% in your opinion. |
@Barryrowe right to the point. It's our pain to implement route guards with Ionic 3 :( Works fine for small apps, but for production ready - nope |
App menus have the same problem.
it didn't, so I had to use ionWillEnter to make the same validations. |
@jgw96 I'm curious if this is being looked at? I know there was talk on other threads around the time I opened this that DeepLinking and routing in general was getting a heavy look, and possible restructure, and that would definitely impact this issue. I haven't seen anything of the sorts, but I may have missed it. Just looking for any feedback the team might have. Thank you. |
Have a look also to this issue #12193 |
I am going to take a look at nav guards soon. Thanks, |
May we return observable as follow??
|
Here my use of ionViewCanEnter(). https://github.com/FazioNico/mean-ionic-ngrx/blob/master/src/decorators/index.ts |
@danbucholtz Any updates on this dan? |
@danbucholtz Any updates? |
@danbucholtz It will be helpful if you share us how long does it take or your opinion to solve this? |
Not sure if this can help, but #12193 was before in different milestones but never closed, now it is on milestones for 4.x so perhaps we will have it there and perhaps also this could be resolved on ionic4. We'll wait for ionic4 and see what they w'll do with router and lazyloading, if we are not satisfied we'll use only there web components with ngx-rocket or plain angular project. IMO |
It seems like ionViewCanEnter(): boolean | Promise<any> {
const isAllowed = false; // some expression here
console.log('TestPage: ionViewCanEnter:', isAllowed);
if (!isAllowed) {
// can't navigate in the same tick
setTimeout(() => {
console.log('TestPage: redirect');
this.navCtrl.setRoot('feedback');
}, 0);
}
return isAllowed;
} |
The above example works fine, but the whole function needs to be replicated in every page. In your AuthProvider:
Now you can just simply call the function in your pages:
|
Hi guys, This is what I did for my firebase project. It works
|
@thegust's approach is very good for individual pages. Anyone got any global solution instead of writing the 'ionViewCanEnter' on every page. |
@HardikDG Wait for Ionic V4 with Angular Routing implementation |
One way is to use async which worked for me.
Then using
|
This issue has been automatically identified as an Ionic 3 issue. We recently moved Ionic 3 to its own repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there. If I've made a mistake, and if this issue is still relevant to Ionic 4, please let the Ionic Framework team know! Thank you for using Ionic! |
Issue moved to: ionic-team/ionic-v3#281 |
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 rejectedionCanViewEnter
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 #11405Leak Objects
If the
ionViewCanEnter
function returns/resolvesfalse
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
Create a project with the tabs template
Implement
ionCanViewenter
in the home page component to return falseImplement
ngOnDestroy
in the home page to console log when it is destroyedAdd a console.log line to the constructor for the home page to show it's being instantiated.
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:
Other information:
Ionic info: (run
ionic info
from a terminal/cmd prompt and paste output below):The text was updated successfully, but these errors were encountered: