Timing issues with ngOnDestroy

[briancodes]

Hi.

I noticed an issue with the ngOnDestroy when moving between pages.

If the embed.js has not finished loading and the component is removed from the view (a new page is loaded), the ngOnDestroy may have removed the window.DISQUS reference which the embed.js code is still trying to access. This can throw null pointer errors - I've only seen it happen once though, so was looking at the code to see if there was any way this could occur.

I think a problem might stem from the fact that the embed.js is reattached each time a component is destroyed and then added back to the view.

I think embed.js script could be attached one single time for the application, and reset() each time a page is loaded. I'm not sure that we should be removing the window.DISQUS reference once it has been set: this.dService.window.DISQUS = undefined;. I'm looking into making some changes locally to test this - where the script is added just once for the application, and reset() is called on each page load - I presume the reset() will look for the <div id="disqus_thread">, will know when I test it. It would also mean less requests being made if it works, which would be a plus

[MurhafSousli]

Hi @briancodes, sorry for the delay.

As far as I know, Disqus does some mysterious requests. therefore, I found it better to remove the script when the component is not used.

Does the error break the app?

[briancodes]

@MurhafSousli I did some further tests around this. The problem is not with the ngx-disqus package.

The error occurs (null pointer error) if the <div id="disqus_thread"></div> is removed while the embed.js is running. More apparent if you throttle the network connection in Chrome DevTools. Just as the embed is added, if you switch quickly to a different page/view it can occur.

This can be closed, as the problem is something that would need to be addressed at an app level e.g. having a single app wide <div id="disqus_thread"></div> element. The app recovers when the next thread is loaded, so even when it occurs, it has no visible effect for the end-user.

[MurhafSousli]

@briancodes Thanks for the feedback!
Closing...

[MurhafSousli]

@briancodes It turned out that there are scripts duplications happening when we load the script again everytime the component initializes, this is fixed in 2.4.0 so the errors are gone.

[briancodes]

@MurhafSousli Hi. Thanks for the update. I did something similar (see below), using ngOnInit for first load, and ngOnChanges for reused routes. Your implementation is cleaner though, having both checks in the ngOnChanges

    ngOnChanges(changes: SimpleChanges): void {
        // Called before ngOnInit, then again when any inputs change
        // Check if relevant change -> may add more inputs, so important to know
        if (this.windowRef.isBrowser &&
            changes &&
            changes['disqusInputs'] &&
            (this.windowRef.window as any).DISQUS
        ) {
            this.reset();
        }
    }

    ngOnInit() {
        if (this.windowRef.isBrowser &&
            this.disqusInputs &&
            !(this.windowRef.window as any).DISQUS
        ) {
            this.attachScript();
        }
    }