Add support for route guards #168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 38 38
Lines 843 869 +26
Branches 113 116 +3
=====================================
+ Hits 843 869 +26
Continue to review full report at Codecov.
|
|
Don't merge this yet, testing some stuff related to deep nested routes. |
|
Nested routes work as expected. Additionally after talking with @Blackbaud-PaulCrowder I'm going to make some changes so that guard creators don't have to |
blackbaud-brandonstirnaman
force-pushed
the
guard-support
branch
5 times, most recently
from
5a82d3d to
72e4a34
Compare
|
With latest commit on this PR, it now supports using both named exports and a default export. Ready for review @Blackbaud-BobbyEarl. |
There was a problem hiding this comment.
LGTM. I would suggest we create additional issues for:
- Documenting an example of this in our
learndocs. @blackbaud-johnly - Create an e2e test to verify
lib/sky-pages-route-generator.js
Outdated
| @@ -114,11 +129,30 @@ function generateDefinitions(routes) { | |||
| function generateDeclarations(routes) { | |||
| const p = indent(1); | |||
| const declarations = routes | |||
| .map(r => `${p}{ path: '${r.routePath}', component: ${r.componentName} }`) | |||
| .map(r => { | |||
| let guard = r.guardPath && r.guardName ? `require('${r.guardPath}').${r.guardName}` : ''; | |||
There was a problem hiding this comment.
Was there not a way to build a list of classes to import like we do in the code where we detect components and add them to the declarations of the NgModule? I'd like to use that same pattern if at all possible. Same for the generateProviders method on line 148.
There was a problem hiding this comment.
Ok. gimme a few minutes.. be ready to review again soon! :)
| } | ||
| }); | ||
|
|
||
| expect(routes.declarations).toContain( |
There was a problem hiding this comment.
This is still checking for .default from your previous implementation. Should this be .Guard instead? There are other instances later in this file, so be sure to address them as well.
There was a problem hiding this comment.
Nope. Its a separate test to verify that .default still works. Basically both paths are explicitly supported now. The test above this one uses the non-default, named class approach with .Guard.
There was a problem hiding this comment.
Removing support for export default entirely now as part of refactoring to use import instead of require
edit: or maybe not. ;)
blackbaud-brandonstirnaman
force-pushed
the
guard-support
branch
from
7acab94 to
e44ba54
Compare
blackbaud-brandonstirnaman
force-pushed
the
guard-support
branch
from
e44ba54 to
e576df4
Compare
blackbaud-brandonstirnaman
force-pushed
the
guard-support
branch
from
e576df4 to
808c60d
Compare
* Initial implementation of cert resolver. * Added check and warning/error if certificate not found. * Switched to using provided sslRoot * Bugfixes and tests. * Cleaned up eslint errors. Fixed e2e tests. * Using new cert installation technique in Travis and Appveyor. * Using initial-commit branch of certs repo. * Slight refactor to not use return in else, based on feedback. * Updated to generated cert technique. * Updated tests. * Fixed tests. * Passing in sslCert and sslKey. Trying OSX too. * Showing specific error. * Debugging * Added missing dash. * Using install instead of trust. * Pointing to latest version of CLI.
Adds support for canActivate/canDeactivate route guards as requested in blackbaud/skyux2#423.
I want to look at refactoring the way route generation works to use childRoutes and all for canActivateChild as well but this is a solid first step to unblock some consumers.