ngOnDestroy crashes SSR apps

[d-moos]

Describe the bug
ngOnDestroy kills SSR due to unchecked usage of document

To Reproduce
destroy an instance of OAuthService on a SSR-app

Expected behavior
Server does not crash

const silentRefreshFrame = document.getElementById(this.silentRefreshIFrameName);

document must be replaced by DOCUMENT of @angular/common

[jeroenheijmans]

Thx for reporting! <3

On one hand I guess the change wouldn't hurt, but there are two things that would make me think twice about this change.

First, if you we'd do this, I suppose all instances of document in the service should use the injected one. So that would be a huge refactor.

Second, I'm not sure how useful it would be to have this service "not crash" with Server Side Rendering? The reason is that I'm not sure what meaningful thing the app could do auth-wise when doing server side rendering? Although I haven't done much SSR myself yet, I'd always expected you'd need to provide a stub-implementation for the OAuthService type as a whole (or more likely: your wrapper service) for meaningful and reliable Server Side functionality?

Those two together makes me wonder if it's worth the change?

[d-moos]

hey jeroen, thanks for replying.

The problem is that at some point the OAuthModule (which includes the OAuthService) will be destroyed and thus can't evade the document call.
All other functions which are using a document reference have to be called manually (or a parent of these) and thus can be avoided if we run the app on a server platform.

Not certainly sure how the second part could work. However since there are SSR examples in the repo documentation (https://github.com/manfredsteyer/angular-oauth2-oidc/blob/master/docs-src/server-side-rendering.md) I feel like there is a SSR usage for the library.

Best,
Daniel

[jeroenheijmans]

will be destroyed and thus can't evade the document call.

An instance would only be destroyed if it was ever onInitiated, I thought? I had always assumed it was relatively easy to inject other types when rendering on the server, at least for crucial services.

All other functions which are using a document reference have to be called manually (or a parent of these) and thus can be avoided if we run the app on a server platform.

Hmm, on one hand you're right. On the other hand, to my personal preference, lack of consistency in a class on how a dependency is used will make future changes much harder.

since there are SSR examples in the repo documentation

Interesting! I didn't know. The links and the blogposts there are 2 years old though, so I'd take them with a pinch of salt.

[d-moos]

hey jeroen,

An instance would only be destroyed if it was ever onInitiated, I thought? I had always assumed it was relatively easy to inject other types when rendering on the server, at least for crucial services.

to be honest I haven't heard or seen any examples of a mock-type injection in a server-side app (besides of injection tokens for browser-objects such as document). happy to read up on it if you can provide me a reference or two.

Hmm, on one hand you're right. On the other hand, to my personal preference, lack of consistency in a class on how a dependency is used will make future changes much harder.

totally agree on this one. it should not be a problem to replace all document references with the injected this.document. will do if we decide to proceed further with the PR.

just want to mention that this part has only been broken 7 days ago due to merging this PR #666 . I'd happily use v8, but since we are using Angular 9 this seems not to be an option.

[WolfspiritM]

I'm also having this issue not directly with SSR but with an app shell which uses a single SSR call on build time to render the shell route. It worked before but now it fails cause of this document call.

I'm now trying to prevent OAuthService from loading at all by splitting server modules and client module, but it would be great if this can be addressed.