Miscellaneous fixes and tweaks #28
Conversation
stevehobbsdev
commented
Aug 12, 2020
stevehobbsdev
commented
- Default redirectUri to window.location.origin
- Unwrap defer around handleRedirectCallback
- Add wildcard support to http interceptor
- Add example in readme for URL wildcard
| it('attach the access token when the configuration uri is a string with a wildcard', fakeAsync(( | ||
| done | ||
| ) => { | ||
| httpClient.get('/basic-api/startsWith?hello=world').subscribe(done); |
There was a problem hiding this comment.
IMO, the use of the wildcard does not have any impact on URLs with or without a query string. The query string should be ignored, not considered when trying to match the endpoint. The path is the only portion we should use to match. Maybe the tests are a bit confusing because all of them include a query string value.
We could improve that to give the right message. A few examples:
- With a rule of
'/api/photos'it will match only exact paths, like/api/photosand/api/photos/(is there any difference between both?? I don't think so). - With a rule of
'/api/photos*'it will match anything that starts with that, like/api/photos,/api/photos/,/api/photos/31,/api/photos/me/summer/2020.
There was a problem hiding this comment.
I updated the tests here with more representative URLs, and refactored out the assertion to a common function.
I updated the regex ones too, even though they're about to be removed in another PR.
| // If the URL ends with an asterisk, match using startsWith. | ||
| if ( | ||
| typeof value === 'string' && | ||
| value.indexOf('*') === value.length - 1 && |
There was a problem hiding this comment.
value.indexOf('*') === value.length - 1
If I recall correctly, indexOf will return -1 when it's not found. And value.length of an empty string is 0. So this line will evaluate to true. The previous one is true as well, because it's still a string, even if empty. And the one below I think it will 💥 when it tries to substring(0, -1).
Can we add a test for this scenario?
There was a problem hiding this comment.
I've added an empty string to the config, as well as putting in a guard for this function that exits early when value is falsy.
| @@ -8,15 +8,20 @@ import { AuthGuard } from './auth.guard'; | |||
| @NgModule() | |||
| export class AuthModule { | |||
| static forRoot(config: AuthConfig): ModuleWithProviders<AuthModule> { | |||
| const defaultedConfig: AuthConfig = { | |||
There was a problem hiding this comment.
nit: can we rename this to defaultConfig, defaultValues or even baseConfig or baseValues ?
There was a problem hiding this comment.
Sure, I'm not precious about the name.
stevehobbsdev
force-pushed
the
misc-fixes
branch
from
057ddf4 to
00664c4
Compare
