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

[router] CanActivate and DI #4112

Closed
cexbrayat opened this Issue Sep 10, 2015 · 89 comments

Comments

Projects
None yet
@cexbrayat
Contributor

cexbrayat commented Sep 10, 2015

The CanActivate hook is available as a decorator.
That's a bit surprising compared to all the other lifecycle hooks which are simple functions of the component. I understand the reason behind that (the hook is called before the component is instanciated), even if I think the API would be simpler if everything was a function...

If things stay like this, I have a question: will it be possible to use DI in the CanActivate hook?

Do you plan to support something like :

@CanActivate((userService: UserService) => userService.isAdmin())
@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Sep 10, 2015

Contributor

I'm not sure if you will be able to do it that way, but you can use the injector in the CanActivate function. Here is a simple example:

http://plnkr.co/edit/zoSAcCkAwQ1TfzxJaiSI?p=preview

Contributor

brandonroberts commented Sep 10, 2015

I'm not sure if you will be able to do it that way, but you can use the injector in the CanActivate function. Here is a simple example:

http://plnkr.co/edit/zoSAcCkAwQ1TfzxJaiSI?p=preview

@tamascsaba

This comment has been minimized.

Show comment
Hide comment
@tamascsaba

tamascsaba Sep 14, 2015

Contributor

You are created new Object from Auth, it is a not the original instance, as I know.

Contributor

tamascsaba commented Sep 14, 2015

You are created new Object from Auth, it is a not the original instance, as I know.

@btford btford added the comp: router label Sep 14, 2015

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Sep 14, 2015

Contributor

Yes, I plan to handle this case.

I think the hooks should be able to augment instructions with bindings to be passed to the component.

Contributor

btford commented Sep 14, 2015

Yes, I plan to handle this case.

I think the hooks should be able to augment instructions with bindings to be passed to the component.

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Sep 14, 2015

Contributor

@btford Thanks for your answer

Contributor

cexbrayat commented Sep 14, 2015

@btford Thanks for your answer

@frederikschubert

This comment has been minimized.

Show comment
Hide comment
@frederikschubert

frederikschubert Nov 4, 2015

Has there been any activity regarding this issue? I would like to inject a router instance into the CanActivate hook of my component which is the root of my login-protected app. So if the user is not authenticated he will be redirected to the login page. I think this is essential for protecting an application.

Or is there any other way to accomplish this behavior?

frederikschubert commented Nov 4, 2015

Has there been any activity regarding this issue? I would like to inject a router instance into the CanActivate hook of my component which is the root of my login-protected app. So if the user is not authenticated he will be redirected to the login page. I think this is essential for protecting an application.

Or is there any other way to accomplish this behavior?

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Nov 4, 2015

Contributor

@frederikschubert I have a simple workaround here. I store the application injector and use it to inject dependencies inside the CanActivate function.

http://plnkr.co/edit/SF8gsYN1SvmUbkosHjqQ?p=preview

Contributor

brandonroberts commented Nov 4, 2015

@frederikschubert I have a simple workaround here. I store the application injector and use it to inject dependencies inside the CanActivate function.

http://plnkr.co/edit/SF8gsYN1SvmUbkosHjqQ?p=preview

@frederikschubert

This comment has been minimized.

Show comment
Hide comment
@frederikschubert

frederikschubert Nov 4, 2015

Ok I see. Thanks! Still getting used to the new DI system. :)

frederikschubert commented Nov 4, 2015

Ok I see. Thanks! Still getting used to the new DI system. :)

@ribizli

This comment has been minimized.

Show comment
Hide comment
@ribizli

ribizli Nov 4, 2015

It is still a workaround, but it works 😀
On 4 Nov 2015 20:42, "Frederik Schubert" notifications@github.com wrote:

Ok I see. Thanks! Still getting used to the new DI system. :)


Reply to this email directly or view it on GitHub
#4112 (comment).

ribizli commented Nov 4, 2015

It is still a workaround, but it works 😀
On 4 Nov 2015 20:42, "Frederik Schubert" notifications@github.com wrote:

Ok I see. Thanks! Still getting used to the new DI system. :)


Reply to this email directly or view it on GitHub
#4112 (comment).

@janneromell

This comment has been minimized.

Show comment
Hide comment
@janneromell

janneromell commented Nov 5, 2015

See this too:
#2965 (comment)

@frederikschubert

This comment has been minimized.

Show comment
Hide comment
@frederikschubert

frederikschubert Nov 5, 2015

@brandonroberts I implemented your workaround and it worked but I discovered some problems. When your evaluation of whether the user is logged in or not depends on Promises or Observables, then the content of the component that is protected in the CanActivate hook is visible for a fraction of a second before the user is redirected to the login component. I do not know if this can be prevented.

@janneromell I also tried implementing your use case where the user gets redirected to the initial requested url after he successfully logged in. This is not possible because the CanActivate hook only receives the ComponentInstruction for the annotated component. To implement this behavior it would be necessary to pass the full Instruction to the hook. @btford Is it planned to implement this?

frederikschubert commented Nov 5, 2015

@brandonroberts I implemented your workaround and it worked but I discovered some problems. When your evaluation of whether the user is logged in or not depends on Promises or Observables, then the content of the component that is protected in the CanActivate hook is visible for a fraction of a second before the user is redirected to the login component. I do not know if this can be prevented.

@janneromell I also tried implementing your use case where the user gets redirected to the initial requested url after he successfully logged in. This is not possible because the CanActivate hook only receives the ComponentInstruction for the annotated component. To implement this behavior it would be necessary to pass the full Instruction to the hook. @btford Is it planned to implement this?

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Nov 5, 2015

Contributor

@frederikschubert can you reproduce the component showing in a plunk? As I understand it, the component behind the CanActivate shouldn't be loaded until after the promise/value is returned as true.

Contributor

brandonroberts commented Nov 5, 2015

@frederikschubert can you reproduce the component showing in a plunk? As I understand it, the component behind the CanActivate shouldn't be loaded until after the promise/value is returned as true.

@frederikschubert

This comment has been minimized.

Show comment
Hide comment
@frederikschubert

frederikschubert Nov 6, 2015

@brandonroberts http://plnkr.co/edit/TdGv0XiExiOvaAFzRWKD
I found a problem with the CanActivate hook. I returned the subscription in my isLoggedIn() that in turn returned whether the user is authenticated or not via an Rx.Observable. Because the subscription is not undefined it is evaluated as true and the content is shown. After the Observable returned false however the user is redirected to the login page.
I think that this can happen quiet often and could be easily prevented by checking the outcome of the CanActivate hook to be exactly true (type safe). Snippet from my plunk:

@CanActivate((to, from) => {
  // This causes the intended behavior.
  // return isLoggedIn() === true;

  // This causes problems
  return isLoggedIn();
})

A fix for this problem would be the following diff.

diff --git a/modules/angular2/src/router/router.ts b/modules/angular2/src/router/router.ts
index b91335c..66e93b7 100644
--- a/modules/angular2/src/router/router.ts
+++ b/modules/angular2/src/router/router.ts
@@ -565,7 +565,7 @@ function canActivateOne(nextInstruction: Instruction,
     var hook = getCanActivateHook(nextInstruction.component.componentType);
     if (isPresent(hook)) {
       return hook(nextInstruction.component,
-                  isPresent(prevInstruction) ? prevInstruction.component : null);
+                  isPresent(prevInstruction) ? prevInstruction.component : null) === true;
     }
     return true;
   });

frederikschubert commented Nov 6, 2015

@brandonroberts http://plnkr.co/edit/TdGv0XiExiOvaAFzRWKD
I found a problem with the CanActivate hook. I returned the subscription in my isLoggedIn() that in turn returned whether the user is authenticated or not via an Rx.Observable. Because the subscription is not undefined it is evaluated as true and the content is shown. After the Observable returned false however the user is redirected to the login page.
I think that this can happen quiet often and could be easily prevented by checking the outcome of the CanActivate hook to be exactly true (type safe). Snippet from my plunk:

@CanActivate((to, from) => {
  // This causes the intended behavior.
  // return isLoggedIn() === true;

  // This causes problems
  return isLoggedIn();
})

A fix for this problem would be the following diff.

diff --git a/modules/angular2/src/router/router.ts b/modules/angular2/src/router/router.ts
index b91335c..66e93b7 100644
--- a/modules/angular2/src/router/router.ts
+++ b/modules/angular2/src/router/router.ts
@@ -565,7 +565,7 @@ function canActivateOne(nextInstruction: Instruction,
     var hook = getCanActivateHook(nextInstruction.component.componentType);
     if (isPresent(hook)) {
       return hook(nextInstruction.component,
-                  isPresent(prevInstruction) ? prevInstruction.component : null);
+                  isPresent(prevInstruction) ? prevInstruction.component : null) === true;
     }
     return true;
   });
@panKt

This comment has been minimized.

Show comment
Hide comment
@panKt

panKt Dec 29, 2015

Is there any workaround for now? I'm trying to inject RouteParams the way @brandonroberts proposed and it does not work this way. Here is updated plunker http://plnkr.co/edit/siMNH53PCuvUBRLk6suc?p=preview
it will say No provider for RouteParams! once you try on inject it (you need to click on Protected Page link)

panKt commented Dec 29, 2015

Is there any workaround for now? I'm trying to inject RouteParams the way @brandonroberts proposed and it does not work this way. Here is updated plunker http://plnkr.co/edit/siMNH53PCuvUBRLk6suc?p=preview
it will say No provider for RouteParams! once you try on inject it (you need to click on Protected Page link)

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Dec 29, 2015

Contributor

@panKt you don't access route params that way. They are already provided asto.params and from.params in my plunker example.

Contributor

brandonroberts commented Dec 29, 2015

@panKt you don't access route params that way. They are already provided asto.params and from.params in my plunker example.

@panKt

This comment has been minimized.

Show comment
Hide comment
@panKt

panKt Dec 29, 2015

OMG, true :) Thanks, @brandonroberts

panKt commented Dec 29, 2015

OMG, true :) Thanks, @brandonroberts

@ribizli

This comment has been minimized.

Show comment
Hide comment
@ribizli

ribizli Dec 29, 2015

@frederikschubert you have to return a promise from the isLoggedIn() method as the documentation says. Observable has a toPromise() method what you can use for this purpose...

ribizli commented Dec 29, 2015

@frederikschubert you have to return a promise from the isLoggedIn() method as the documentation says. Observable has a toPromise() method what you can use for this purpose...

@danikenan

This comment has been minimized.

Show comment
Hide comment
@danikenan

danikenan Jan 9, 2016

It's nice that we can use workarounds like the ones above(@brandonroberts thanks), but this needs to be addressed by the core and simplified. Decorators such as CanActivate must be able to interact with the DI, namely be injected with their dependencies.
They are almost useless otherwise, or requires tons of complicating workarounds...

danikenan commented Jan 9, 2016

It's nice that we can use workarounds like the ones above(@brandonroberts thanks), but this needs to be addressed by the core and simplified. Decorators such as CanActivate must be able to interact with the DI, namely be injected with their dependencies.
They are almost useless otherwise, or requires tons of complicating workarounds...

@Schnueggel

This comment has been minimized.

Show comment
Hide comment
@Schnueggel

Schnueggel Jan 11, 2016

@brandonroberts thanks. Best solution so far.

Schnueggel commented Jan 11, 2016

@brandonroberts thanks. Best solution so far.

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Jan 12, 2016

Contributor

The plunker example has been updated to beta.1

Contributor

brandonroberts commented Jan 12, 2016

The plunker example has been updated to beta.1

@nielslbeck

This comment has been minimized.

Show comment
Hide comment
@nielslbeck

nielslbeck Jan 12, 2016

Great workaround, @brandonroberts.

But in the real world I guess you would have an ngOnInit in Auth.ts where you would check in local storage or a remote server if you are already logged in, right? But ngOnInit isn't executed by the injector and thus loggedIn isn't updated to reflect the correct state.

So we have to have another workaround where we manually call ngOnInit. But ngOnInit should only be called once so it has to remember whether it's been called before.

Workaround on workaround... not pretty!

I hope we get DI for decorators.

nielslbeck commented Jan 12, 2016

Great workaround, @brandonroberts.

But in the real world I guess you would have an ngOnInit in Auth.ts where you would check in local storage or a remote server if you are already logged in, right? But ngOnInit isn't executed by the injector and thus loggedIn isn't updated to reflect the correct state.

So we have to have another workaround where we manually call ngOnInit. But ngOnInit should only be called once so it has to remember whether it's been called before.

Workaround on workaround... not pretty!

I hope we get DI for decorators.

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Jan 12, 2016

Contributor

@nielslbeck Sure. There are plenty of ways to achieve the same outcome. The CanActivate function keeps your component from being loaded at all until it returns true. ngOnInit will never be called if the CanActivate function returns false because the component was never loaded.

Contributor

brandonroberts commented Jan 12, 2016

@nielslbeck Sure. There are plenty of ways to achieve the same outcome. The CanActivate function keeps your component from being loaded at all until it returns true. ngOnInit will never be called if the CanActivate function returns false because the component was never loaded.

@nolazybits

This comment has been minimized.

Show comment
Hide comment
@nolazybits

nolazybits Jan 14, 2016

@brandonroberts thanks for the workaround in the meantime

nolazybits commented Jan 14, 2016

@brandonroberts thanks for the workaround in the meantime

@crowebird

This comment has been minimized.

Show comment
Hide comment
@crowebird

crowebird Jan 14, 2016

@brandonroberts +1 +1 +1

By the doc comments for ComponentInstruction, the instance of ComponentInstruction is meant to be immutable, but I am performing a query in CanActivate to see if data exists for the route. In performing that query I get data that I want to show on the page. Rather than re-query once I get inside the class I need a way to pass that data inside. To do this I am adding a key to the instance of componentinstruction with my data and retrieving it inside the routerOnActivate function.

Is there a better way?

crowebird commented Jan 14, 2016

@brandonroberts +1 +1 +1

By the doc comments for ComponentInstruction, the instance of ComponentInstruction is meant to be immutable, but I am performing a query in CanActivate to see if data exists for the route. In performing that query I get data that I want to show on the page. Rather than re-query once I get inside the class I need a way to pass that data inside. To do this I am adding a key to the instance of componentinstruction with my data and retrieving it inside the routerOnActivate function.

Is there a better way?

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Jan 14, 2016

Contributor

@crowebird Another terrible workaround(disclaimer) is extending the route data in the CanActivate. Then you can inject RouteData into your component and use it in the constructor instead of the routerOnActivate.

http://plnkr.co/edit/wWG8XB6SX2OPX5OklLfj?p=preview

A better way would be to use a service for sharing data. You pass data to the service in the CanActivate, and inject that service into your component to retrieve the data.

Contributor

brandonroberts commented Jan 14, 2016

@crowebird Another terrible workaround(disclaimer) is extending the route data in the CanActivate. Then you can inject RouteData into your component and use it in the constructor instead of the routerOnActivate.

http://plnkr.co/edit/wWG8XB6SX2OPX5OklLfj?p=preview

A better way would be to use a service for sharing data. You pass data to the service in the CanActivate, and inject that service into your component to retrieve the data.

@crowebird

This comment has been minimized.

Show comment
Hide comment
@crowebird

crowebird Jan 14, 2016

@brandonroberts - Interesting! I like that better than having to get the data through routerOnActivate.

Will have to work for now and I can just add a unique key to RouteData data instead of overwriting the whole thing.

crowebird commented Jan 14, 2016

@brandonroberts - Interesting! I like that better than having to get the data through routerOnActivate.

Will have to work for now and I can just add a unique key to RouteData data instead of overwriting the whole thing.

@ThiagoT1

This comment has been minimized.

Show comment
Hide comment
@ThiagoT1

ThiagoT1 Jan 16, 2016

I need to recover a singleton instance while at the CanActivate handler.

How ugly is this?

ng.router.CanActivate(
    function () {
        return singletonService.Service.getInstance().getPromise();
    }
)(AppComponent);

singletonService.Service.getInstance could be set inside its constructor.

ThiagoT1 commented Jan 16, 2016

I need to recover a singleton instance while at the CanActivate handler.

How ugly is this?

ng.router.CanActivate(
    function () {
        return singletonService.Service.getInstance().getPromise();
    }
)(AppComponent);

singletonService.Service.getInstance could be set inside its constructor.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Jan 17, 2016

Contributor

@ThiagoT1 actually, dependencies are already singletons by default as long as they are a) created by Angular's DI and b) injected in the same injector container.

Contributor

PascalPrecht commented Jan 17, 2016

@ThiagoT1 actually, dependencies are already singletons by default as long as they are a) created by Angular's DI and b) injected in the same injector container.

@clemcke

This comment has been minimized.

Show comment
Hide comment
@clemcke

clemcke Jan 26, 2016

I wrote a spec test for the appInjector() solution. (Based off of @brandonroberts implementation) https://gist.github.com/clemcke/c5902028a1b0934b03d9

clemcke commented Jan 26, 2016

I wrote a spec test for the appInjector() solution. (Based off of @brandonroberts implementation) https://gist.github.com/clemcke/c5902028a1b0934b03d9

@astopo

This comment has been minimized.

Show comment
Hide comment
@astopo

astopo Feb 15, 2016

+1 @brandonroberts thanks for the plunker! Would prefer not to inject services in bootstrap, but is a good work around for now.

astopo commented Feb 15, 2016

+1 @brandonroberts thanks for the plunker! Would prefer not to inject services in bootstrap, but is a good work around for now.

@swunderlich

This comment has been minimized.

Show comment
Hide comment
@swunderlich

swunderlich Feb 20, 2016

@brandonroberts Hi, I try to use your plunker http://plnkr.co/edit/SF8gsYN1SvmUbkosHjqQ?p=preview but I always end up with:

Cannot resolve all parameters for 'function (injector) {'(?). Make sure that all the parameters are decorated with Inject or have valid type annotations and that 'function (injector) {' is decorated with Injectable.
angular2-polyfills.js:1243 Error: Cannot read property 'getOptional' of undefined(…)Zone.run @ angular2-polyfills.js:1243zoneBoundFn @ angular2-polyfills.js:1220lib$es6$promise$$internal$$tryCatch @ angular2-polyfills.js:468lib$es6$promise$$internal$$invokeCallback @ angular2-polyfills.js:480lib$es6$promise$$internal$$publish @ angular2-polyfills.js:451lib$es6$promise$$internal$$publishRejection @ angular2-polyfills.js:401(anonymous function) @ angular2-polyfills.js:123Zone.run @ angular2-polyfills.js:1243zoneBoundFn @ angular2-polyfills.js:1220lib$es6$promise$asap$$flush @ angular2-polyfills.js:262

Any ideas or somebody facing the same issue?

swunderlich commented Feb 20, 2016

@brandonroberts Hi, I try to use your plunker http://plnkr.co/edit/SF8gsYN1SvmUbkosHjqQ?p=preview but I always end up with:

Cannot resolve all parameters for 'function (injector) {'(?). Make sure that all the parameters are decorated with Inject or have valid type annotations and that 'function (injector) {' is decorated with Injectable.
angular2-polyfills.js:1243 Error: Cannot read property 'getOptional' of undefined(…)Zone.run @ angular2-polyfills.js:1243zoneBoundFn @ angular2-polyfills.js:1220lib$es6$promise$$internal$$tryCatch @ angular2-polyfills.js:468lib$es6$promise$$internal$$invokeCallback @ angular2-polyfills.js:480lib$es6$promise$$internal$$publish @ angular2-polyfills.js:451lib$es6$promise$$internal$$publishRejection @ angular2-polyfills.js:401(anonymous function) @ angular2-polyfills.js:123Zone.run @ angular2-polyfills.js:1243zoneBoundFn @ angular2-polyfills.js:1220lib$es6$promise$asap$$flush @ angular2-polyfills.js:262

Any ideas or somebody facing the same issue?

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Feb 20, 2016

Contributor

@swunderlich I just tried the plunker and it worked fine for me. That error means you have requested something that haven't been provided to dependency injection. The injector workaround uses the providers that are in your bootstrap, so if you haven't placed it there, you won't be able to get it using the injector. If you make a plunker of the error we'll be able to diagnose it better.

Contributor

brandonroberts commented Feb 20, 2016

@swunderlich I just tried the plunker and it worked fine for me. That error means you have requested something that haven't been provided to dependency injection. The injector workaround uses the providers that are in your bootstrap, so if you haven't placed it there, you won't be able to get it using the injector. If you make a plunker of the error we'll be able to diagnose it better.

@swunderlich

This comment has been minimized.

Show comment
Hide comment
@wardbell

This comment has been minimized.

Show comment
Hide comment
@wardbell

wardbell Mar 12, 2016

Contributor

I started exploring this again. I came up with my own preferred approach. But frankly, we're really stuck here. We can handle some scenarios with these tricks but not some of the most important ones.

The crux is that we have no opportunity to use the router or the injector that operates at the level of the target component. You're ok if you can reach what you need with the root injector and the root router. But if you need something ... let's say a data service ... that isn't known to the root injector, you are toast.

In all of the scenarios above, you've made a critical assumptions:

  • you can use the root router to go where you want to go (namely the login page).
  • the service you need that tells you if you're logged in is accessible from the root injector

Good for you. But if I've got services that are only available in feature areas that I reach via a child router, I'm hosed. Why? Because the services I need aren't defined at the root injector level. If I try to resolve them at the root level, it will create a service instance at the root level ... which is not the same service instance that a child component will inject later. That's ok if the service is stateless; it's not ok if the service is stateful (e.g., if it caches).

This is just a mess. AFAIK, it won't be addressed before v.1.

At least you have a way to handle the auth requirements.

Contributor

wardbell commented Mar 12, 2016

I started exploring this again. I came up with my own preferred approach. But frankly, we're really stuck here. We can handle some scenarios with these tricks but not some of the most important ones.

The crux is that we have no opportunity to use the router or the injector that operates at the level of the target component. You're ok if you can reach what you need with the root injector and the root router. But if you need something ... let's say a data service ... that isn't known to the root injector, you are toast.

In all of the scenarios above, you've made a critical assumptions:

  • you can use the root router to go where you want to go (namely the login page).
  • the service you need that tells you if you're logged in is accessible from the root injector

Good for you. But if I've got services that are only available in feature areas that I reach via a child router, I'm hosed. Why? Because the services I need aren't defined at the root injector level. If I try to resolve them at the root level, it will create a service instance at the root level ... which is not the same service instance that a child component will inject later. That's ok if the service is stateless; it's not ok if the service is stateful (e.g., if it caches).

This is just a mess. AFAIK, it won't be addressed before v.1.

At least you have a way to handle the auth requirements.

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Mar 12, 2016

Contributor

Care to share what your preferred approach is @wardbell?

Contributor

brandonroberts commented Mar 12, 2016

Care to share what your preferred approach is @wardbell?

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Mar 12, 2016

Contributor

I opened this issue 6 months ago, and it has come up again and again in the projects I've seen, exactly with the problem explained by @wardbell.

I really hope the team will consider it for v1

Contributor

cexbrayat commented Mar 12, 2016

I opened this issue 6 months ago, and it has come up again and again in the projects I've seen, exactly with the problem explained by @wardbell.

I really hope the team will consider it for v1

@robwormald

This comment has been minimized.

Show comment
Hide comment
@robwormald
Member

robwormald commented Mar 12, 2016

note this is included in the RC candidate milestone: https://github.com/angular/angular/milestones/Angular%202%20Release%20Candidate

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Mar 12, 2016

Contributor

@cexbrayat @wardbell can one of you explain how you expect this to work? Do you want the providers and/or dependencies from the route component that hasn't been activated yet to be available to the CanActivate and then that same instances to be provided to the component once the CanActivate passes?

Contributor

brandonroberts commented Mar 12, 2016

@cexbrayat @wardbell can one of you explain how you expect this to work? Do you want the providers and/or dependencies from the route component that hasn't been activated yet to be available to the CanActivate and then that same instances to be provided to the component once the CanActivate passes?

@wardbell

This comment has been minimized.

Show comment
Hide comment
@wardbell

wardbell Mar 12, 2016

Contributor

@brandonroberts I think my answer is "yes". I think that's reasonable as those are resources that will have to become available to instantiate that component.

I think that implies creating the injector instance (or proto) and scoped service instances that will be injected into the target component if it passes the CanActivate test.

Of course, we have to account for that in a (future) world of just-in-time, dynamically loaded "feature modules". It's not just about lazy loading components; it's about lazy loading anything.

Frankly, I keep coming back to my suspicion that CanActivate is a well-intentioned mistake. It seemed like a good (and clever) idea to check a class method. No worries about what the freshly created instance might run off and do on its own. But we've since learned how that path is fraught with complications.

These complications would disappear and clarity would flow if it were an instance method, executed after the component is constructed and before any other lifecycle hook! Ideally before any attempt by A2 to evaluate bindings to the component or do anything with the component.

Our job, as developers of the component, is to make sure that component construction is cheap and safe. Our component constructor must do no work. No running off to the database. It should simply wire up component values. And that's what I recommend to everyone anyway.

There is such a candidate router method, OnActivate. I'd say "forget about that CanActivate and just use OnActivate." Unfortunately, as yet I am unable to get that method to work as advertised. That's an issue I'm exploring separately.

p.s.: @brandonroberts - I'd like to talk to you by email but don't know how to reach you. You can email me at my github contributor address. Thanks!

Contributor

wardbell commented Mar 12, 2016

@brandonroberts I think my answer is "yes". I think that's reasonable as those are resources that will have to become available to instantiate that component.

I think that implies creating the injector instance (or proto) and scoped service instances that will be injected into the target component if it passes the CanActivate test.

Of course, we have to account for that in a (future) world of just-in-time, dynamically loaded "feature modules". It's not just about lazy loading components; it's about lazy loading anything.

Frankly, I keep coming back to my suspicion that CanActivate is a well-intentioned mistake. It seemed like a good (and clever) idea to check a class method. No worries about what the freshly created instance might run off and do on its own. But we've since learned how that path is fraught with complications.

These complications would disappear and clarity would flow if it were an instance method, executed after the component is constructed and before any other lifecycle hook! Ideally before any attempt by A2 to evaluate bindings to the component or do anything with the component.

Our job, as developers of the component, is to make sure that component construction is cheap and safe. Our component constructor must do no work. No running off to the database. It should simply wire up component values. And that's what I recommend to everyone anyway.

There is such a candidate router method, OnActivate. I'd say "forget about that CanActivate and just use OnActivate." Unfortunately, as yet I am unable to get that method to work as advertised. That's an issue I'm exploring separately.

p.s.: @brandonroberts - I'd like to talk to you by email but don't know how to reach you. You can email me at my github contributor address. Thanks!

@wardbell

This comment has been minimized.

Show comment
Hide comment
@wardbell

wardbell Mar 12, 2016

Contributor

FYI, I just filed issue #7576 regarding my troubles with OnActivate.

Contributor

wardbell commented Mar 12, 2016

FYI, I just filed issue #7576 regarding my troubles with OnActivate.

@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts Mar 14, 2016

Contributor

Thanks @wardbell for the explanation. I agree that the constructor should be thin and be used for simply wiring things up. Its an easy trap for developers to fall into making the constructor very heavy and putting logic there. The OnActivate method could work if you push the routes down a level to another outlet also. I can see the benefit on wanted a route component to never be created if the check fails, but use cases like yours are definitely needed.

Contributor

brandonroberts commented Mar 14, 2016

Thanks @wardbell for the explanation. I agree that the constructor should be thin and be used for simply wiring things up. Its an easy trap for developers to fall into making the constructor very heavy and putting logic there. The OnActivate method could work if you push the routes down a level to another outlet also. I can see the benefit on wanted a route component to never be created if the check fails, but use cases like yours are definitely needed.

@IsaacBorrero

This comment has been minimized.

Show comment
Hide comment
@IsaacBorrero

IsaacBorrero Mar 19, 2016

Just adding my request for this feature. I am happy to see it will be addressed in RC1. I need to verify a user is authorized before navigating to various routes.

IsaacBorrero commented Mar 19, 2016

Just adding my request for this feature. I am happy to see it will be addressed in RC1. I need to verify a user is authorized before navigating to various routes.

@borispk

This comment has been minimized.

Show comment
Hide comment
@borispk

borispk Mar 26, 2016

I just wonder inside CanActivate calling a call back function, say calling an API through http request. As the http returns is a call back function to determine the page is show or not depends on the return value. However, after calling the http request API, the async frature would go to the final step of CanActivate function. As a result, the activate behavior does not depend on the API return value. Is there any work around?

borispk commented Mar 26, 2016

I just wonder inside CanActivate calling a call back function, say calling an API through http request. As the http returns is a call back function to determine the page is show or not depends on the return value. However, after calling the http request API, the async frature would go to the final step of CanActivate function. As a result, the activate behavior does not depend on the API return value. Is there any work around?

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 26, 2016

Contributor

@borispk can you provide a Plunker that shows what you try to accomplish? Just completing the Promise when the response is processed shouldn't be difficult.

Contributor

zoechi commented Mar 26, 2016

@borispk can you provide a Plunker that shows what you try to accomplish? Just completing the Promise when the response is processed shouldn't be difficult.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Mar 26, 2016

Contributor

@wardbell can you please share your solution for this? The one you've
mentioned? I think it's good to see and evaluate different approaches as we
don't have a go-to solution yet.

Thanks!

On Sat, Mar 26, 2016, 1:02 PM Günter Zöchbauer notifications@github.com
wrote:

@borispk https://github.com/borispk can you provide a Plunker that
shows what you try to accomplish? Just completing the Promise when the
response is processed shouldn't be difficult.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#4112 (comment)

Contributor

PascalPrecht commented Mar 26, 2016

@wardbell can you please share your solution for this? The one you've
mentioned? I think it's good to see and evaluate different approaches as we
don't have a go-to solution yet.

Thanks!

On Sat, Mar 26, 2016, 1:02 PM Günter Zöchbauer notifications@github.com
wrote:

@borispk https://github.com/borispk can you provide a Plunker that
shows what you try to accomplish? Just completing the Promise when the
response is processed shouldn't be difficult.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#4112 (comment)

@wardbell

This comment has been minimized.

Show comment
Hide comment
@wardbell

wardbell Mar 27, 2016

Contributor

@PascalPrecht @brandonroberts @cexbrayat and others: you may find interesting the design "issue" I just created as #7784 which explores my (limited) understanding of what might be.

Contributor

wardbell commented Mar 27, 2016

@PascalPrecht @brandonroberts @cexbrayat and others: you may find interesting the design "issue" I just created as #7784 which explores my (limited) understanding of what might be.

@borispk

This comment has been minimized.

Show comment
Hide comment
@borispk

borispk Mar 29, 2016

Thank @zoechi and others, Here is my code for the captioned problem. See if you have any advice.

@CanActivate(next: ComponentInstruction, prev: ComponentInstruction) => {
let rtn = funHttpAPI(username, (allowAccess) => {
             return allowAccess;     // callBack function of funHttpAPI, Suppose allowAccess = true, then CanActivate return true, or otherwise.
      });
return rtn;

 return false;     // the page hold up here if return false, otherwise, the page is opened and not wait for funHttpAPI return value.
})

borispk commented Mar 29, 2016

Thank @zoechi and others, Here is my code for the captioned problem. See if you have any advice.

@CanActivate(next: ComponentInstruction, prev: ComponentInstruction) => {
let rtn = funHttpAPI(username, (allowAccess) => {
             return allowAccess;     // callBack function of funHttpAPI, Suppose allowAccess = true, then CanActivate return true, or otherwise.
      });
return rtn;

 return false;     // the page hold up here if return false, otherwise, the page is opened and not wait for funHttpAPI return value.
})
@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Mar 29, 2016

Contributor

@borispk I think you should one of the channels listed in https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question with a Plunker that allows to reproduce if possible.

Contributor

zoechi commented Mar 29, 2016

@borispk I think you should one of the channels listed in https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question with a Plunker that allows to reproduce if possible.

@kemsky

This comment has been minimized.

Show comment
Hide comment
@kemsky

kemsky Apr 2, 2016

In fact, we need injection in every decorator or API should be reworked. Imagine you have loader property set in your route definition, obviously you may want to access model, service or whatever else to decide which component to load. Otherwise developers would have to implement there own routing using Dynamic Component Loader or various static hacks as shown above.

kemsky commented Apr 2, 2016

In fact, we need injection in every decorator or API should be reworked. Imagine you have loader property set in your route definition, obviously you may want to access model, service or whatever else to decide which component to load. Otherwise developers would have to implement there own routing using Dynamic Component Loader or various static hacks as shown above.

@ribizli

This comment has been minimized.

Show comment
Hide comment
@ribizli

ribizli Apr 5, 2016

Hi,

I've read an article in the last days, where a custom router-outlet was used to control access to the routes. This component has an activate method where things can happen. DI works as usual in components.

What do you think about that solution? Can it replace the @CanActivate logic?

ribizli commented Apr 5, 2016

Hi,

I've read an article in the last days, where a custom router-outlet was used to control access to the routes. This component has an activate method where things can happen. DI works as usual in components.

What do you think about that solution? Can it replace the @CanActivate logic?

@divined

This comment has been minimized.

Show comment
Hide comment
@divined

divined Apr 11, 2016

and what about role based routes?

@CanActivate(roleCheck($roleNames[]))

divined commented Apr 11, 2016

and what about role based routes?

@CanActivate(roleCheck($roleNames[]))

@syndicatedshannon

This comment has been minimized.

Show comment
Hide comment
@syndicatedshannon

syndicatedshannon May 2, 2016

I'd like to mention that I'm using this ticket feature (CanActivate + DI) as a workaround for another issue; Route Dependency Resolution. I just discovered this feature (CanActivate + DI) doesn't fully meet my needs for that workaround because the CanActivate hook is not run for AuxRoute Components.

I noted this problem in its correct home, #4015 . This may not be a problem for more legitimate uses of CanActivate, but I thought I'd mention it just to be a pain, or in case it helps. ;)

syndicatedshannon commented May 2, 2016

I'd like to mention that I'm using this ticket feature (CanActivate + DI) as a workaround for another issue; Route Dependency Resolution. I just discovered this feature (CanActivate + DI) doesn't fully meet my needs for that workaround because the CanActivate hook is not run for AuxRoute Components.

I noted this problem in its correct home, #4015 . This may not be a problem for more legitimate uses of CanActivate, but I thought I'd mention it just to be a pain, or in case it helps. ;)

@DrClick

This comment has been minimized.

Show comment
Hide comment
@DrClick

DrClick May 9, 2016

So I found an interesting solution the problem of allowing routes based on an auth service. Using a directive with transclusion. This solves a lot of problems. mainly when routing to a protected component, once you are logged in, you are at the component you should be at. and your route is initialized while the user logs in. This may seem like a disadvantage, but seems to work well for me.

import {Component, OnDestroy, OnInit } from '@angular/core';
import {AuthService} from './auth.service';
import {ROUTER_DIRECTIVES, Router, OnActivate} from "@angular/router-deprecated";
import {Location} from "@angular/common";

@Component({
    selector: 'protected',
    template: `
        <div class="jumbotron" *ngIf='!isAuthenticated()'>
        <div class="container">
            <h1 class="display-3">Welcome to the dark side</h1>
            <p class="lead">You must first <a (click)="login()">login</a>
            to access this resource</p>
        </div>
        </div>
        <div *ngIf='isAuthenticated()'>
            <ng-content></ng-content>
        </div>
    `
})

export class ProtectedComponent implements OnInit, OnDestroy {
    private logoutSubscription: any = null;

    constructor(
        private authService: AuthService,
        private router: Router,
        private location: Location) {
    }

    isAuthenticated() {
        return this.authService.isAuthenticated();
    }

    ngOnInit() {
        this.logoutSubscription = this.authService.subscribe((val) => {
            if (!val.authenticated) {
                this.location.replaceState('/');
                this.router.navigate(['Home']);
            }
        });
    }

    ngOnDestroy() {
        if (this.logoutSubscription != null) {
            this.logoutSubscription.unsubscribe();
        }
    }

    login() {
        this.authService.doLogin();
    }
}

then in my component I want to protect

...
import {ProtectedComponent} from "../shared/protected.component";


@Component({
   ...
    template: `
        <protected><router-outlet></router-outlet></protected>
      `,
   ...
    directives: [RouterOutlet, ProtectedComponent],
    ...
})


export class ModelMetricsComponent {

}


DrClick commented May 9, 2016

So I found an interesting solution the problem of allowing routes based on an auth service. Using a directive with transclusion. This solves a lot of problems. mainly when routing to a protected component, once you are logged in, you are at the component you should be at. and your route is initialized while the user logs in. This may seem like a disadvantage, but seems to work well for me.

import {Component, OnDestroy, OnInit } from '@angular/core';
import {AuthService} from './auth.service';
import {ROUTER_DIRECTIVES, Router, OnActivate} from "@angular/router-deprecated";
import {Location} from "@angular/common";

@Component({
    selector: 'protected',
    template: `
        <div class="jumbotron" *ngIf='!isAuthenticated()'>
        <div class="container">
            <h1 class="display-3">Welcome to the dark side</h1>
            <p class="lead">You must first <a (click)="login()">login</a>
            to access this resource</p>
        </div>
        </div>
        <div *ngIf='isAuthenticated()'>
            <ng-content></ng-content>
        </div>
    `
})

export class ProtectedComponent implements OnInit, OnDestroy {
    private logoutSubscription: any = null;

    constructor(
        private authService: AuthService,
        private router: Router,
        private location: Location) {
    }

    isAuthenticated() {
        return this.authService.isAuthenticated();
    }

    ngOnInit() {
        this.logoutSubscription = this.authService.subscribe((val) => {
            if (!val.authenticated) {
                this.location.replaceState('/');
                this.router.navigate(['Home']);
            }
        });
    }

    ngOnDestroy() {
        if (this.logoutSubscription != null) {
            this.logoutSubscription.unsubscribe();
        }
    }

    login() {
        this.authService.doLogin();
    }
}

then in my component I want to protect

...
import {ProtectedComponent} from "../shared/protected.component";


@Component({
   ...
    template: `
        <protected><router-outlet></router-outlet></protected>
      `,
   ...
    directives: [RouterOutlet, ProtectedComponent],
    ...
})


export class ModelMetricsComponent {

}


@jkyoutsey

This comment has been minimized.

Show comment
Hide comment
@jkyoutsey

jkyoutsey May 19, 2016

Does anyone have an example of how to unit test a class that depends on the app injector? Since jasmine/karma abstracts the bootstrap process, how do I create/reference the injector in my unit test?

In main.ts

bootstrap(AppComponent, [
    ...
]).then((appRef: ComponentRef<AppComponent>) => {
        AppInjector.getInstance().setInjector(appRef.injector);
    },
    error => console.log(error)
    );

In class constructor:

        let injector: AppInjector = AppInjector.getInstance();
        this.uiStateService = injector.getInjector().get(UiStateService);

All works. But unit tests can't find the injector.

---- EDIT: I'll answer my own question:
In your spec:

import {TestInjector} from '@angular/core/testing';
...
beforeEach(() => {
    var testInjector: TestInjector = new TestInjector();
    testInjector.addProviders([
        // providers needed in CanActivate components
    ]);
    testInjector.createInjector();
    AppInjector.getInstance().setInjector(testInjector);
});

jkyoutsey commented May 19, 2016

Does anyone have an example of how to unit test a class that depends on the app injector? Since jasmine/karma abstracts the bootstrap process, how do I create/reference the injector in my unit test?

In main.ts

bootstrap(AppComponent, [
    ...
]).then((appRef: ComponentRef<AppComponent>) => {
        AppInjector.getInstance().setInjector(appRef.injector);
    },
    error => console.log(error)
    );

In class constructor:

        let injector: AppInjector = AppInjector.getInstance();
        this.uiStateService = injector.getInjector().get(UiStateService);

All works. But unit tests can't find the injector.

---- EDIT: I'll answer my own question:
In your spec:

import {TestInjector} from '@angular/core/testing';
...
beforeEach(() => {
    var testInjector: TestInjector = new TestInjector();
    testInjector.addProviders([
        // providers needed in CanActivate components
    ]);
    testInjector.createInjector();
    AppInjector.getInstance().setInjector(testInjector);
});
@brandonroberts

This comment has been minimized.

Show comment
Hide comment
@brandonroberts

brandonroberts May 19, 2016

Contributor

@jkyoutsey see higher up in the thread: #4112 (comment)

Contributor

brandonroberts commented May 19, 2016

@jkyoutsey see higher up in the thread: #4112 (comment)

@nolazybits

This comment has been minimized.

Show comment
Hide comment
@nolazybits

nolazybits Jun 16, 2016

Seems that this is now working as @brandonroberts said on gitter.
Link of working example is http://plnkr.co/edit/gQcyGu?p=preview

nolazybits commented Jun 16, 2016

Seems that this is now working as @brandonroberts said on gitter.
Link of working example is http://plnkr.co/edit/gQcyGu?p=preview

@nolazybits

This comment has been minimized.

Show comment
Hide comment
@nolazybits

nolazybits Jun 16, 2016

Ooops, that is for the new Router v3 not the deprecated v2. Yet people will be glad to see it works in v3 :)

nolazybits commented Jun 16, 2016

Ooops, that is for the new Router v3 not the deprecated v2. Yet people will be glad to see it works in v3 :)

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Jun 22, 2016

Contributor

I guess this can be closed now the new router is available and supports this use case.

Contributor

zoechi commented Jun 22, 2016

I guess this can be closed now the new router is available and supports this use case.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jun 22, 2016

Contributor

Please reopen if still a problem (thanks @zoechi)

Contributor

vicb commented Jun 22, 2016

Please reopen if still a problem (thanks @zoechi)

@vicb vicb closed this Jun 22, 2016

@Chabane

This comment has been minimized.

Show comment
Hide comment
@Chabane

Chabane Sep 9, 2016

I got this error with rc6 :

Uncaught TypeError: core_1.provide is not a function

Chabane commented Sep 9, 2016

I got this error with rc6 :

Uncaught TypeError: core_1.provide is not a function

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Sep 10, 2016

Contributor

provide is obsolete, use plain object notation

Contributor

Delagen commented Sep 10, 2016

provide is obsolete, use plain object notation

@Chabane

This comment has been minimized.

Show comment
Hide comment
@Chabane

Chabane Sep 10, 2016

@Delagen Yes, but my only provider declaration is dedicated for ng2Translate

 TranslateModule.forRoot({
            provide: TranslateLoader,
            useFactory: (http: Http) => new TranslateStaticLoader(http, 'assets/i18n', '.json'),
            deps: [Http]
        })

So, the problem it was in another package ^^. It's solved now. Thanks.

Chabane commented Sep 10, 2016

@Delagen Yes, but my only provider declaration is dedicated for ng2Translate

 TranslateModule.forRoot({
            provide: TranslateLoader,
            useFactory: (http: Http) => new TranslateStaticLoader(http, 'assets/i18n', '.json'),
            deps: [Http]
        })

So, the problem it was in another package ^^. It's solved now. Thanks.

ruchirsachdeva pushed a commit to ruchirsachdeva/angular-web-client that referenced this issue Feb 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment