-
Notifications
You must be signed in to change notification settings - Fork 431
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
moving to strict mode #1848
moving to strict mode #1848
Conversation
…m/damienbod/angular-auth-oidc-client into fabiangosebrink-fix/strict-mode-1
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
1 similar comment
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
I got some time later this week, I'll try to review this PR. |
I don't think it is possible to review all files. @damienbod will test, I also did run a few test, worked fine so far. |
Yea, I'll review some of the files within the library source, and also do a few tests. |
That is even more than I can ask for. That will be perfect. Thank you! |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
1 similar comment
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @FabianGosebrink ! 🏆
It also compiles and works on the project I'm working on.
I do got two questions/comments though:
- I notice that
any
is used here and there, I can pick this up after this PR is merged - I also noticed that some operations where the config is used uses the following pattern
tokenRefreshInSeconds ?? 0
(I assume this is for if the config option isnull
orundefined
). Is this correct, because the docs/JSDocs mention a default value. For both cases can something be said, I take the assumption that for these cases we expect the user to configure the option with thenull
value?
projects/angular-auth-oidc-client/src/lib/callback/interval.service.ts
Outdated
Show resolved
Hide resolved
…rvice.ts Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
That would be nice, yes. Happy to help.
Ah, we should use the default value of 0. However, what if the user specifies/overwrites it with undefined or null? Well, we could let it crash then, because the value is not supported. Thoughts? |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
I believe this can be added in the future with a new major release. |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-hill-002425310-1848.centralus.azurestaticapps.net |
I think we can merge then, can't we? |
No description provided.