disablePageUnloadEvents lost somewhere

[twDuke]

ApplicationInsights-JS/AISKU/src/AISku.ts

Lines 332 to 344 in b89849b

	let analyticsPlugin = appInsightsInstance.appInsights;
	let theConfig = analyticsPlugin.config;
	if (!_houseKeepingNamespace) {
	_houseKeepingNamespace = mergeEvtNamespace(_evtNamespace, _core.evtNamespace && _core.evtNamespace());
	}
	// Will be recalled if any referenced config properties change
	_addUnloadHook(onConfigChange(_config, () => {
	// As we could get recalled, remove any previously registered event handlers first
	_removePageEventHandlers();
	let excludePageUnloadEvents = theConfig.disablePageUnloadEvents;

Seems the disablePageUnloadEvents config is lost on the way somewhere so all unload event handlers are registered even if you try and disable them.

appInsights = new ApplicationInsights({ config: { disablePageUnloadEvents: ["unload", "beforeunload"], }, });
in AISku.ts line 332
let analyticsPlugin = appInsightsInstance.appInsights;
appInsightsInstance.config.disablePageUnloadEvents has the expected config value
but
appInsightsInstance.appInsights.config does not, and as thats the one that is assigned to theConfig the expected value is not used.

previously solved in #1683 #1684

[MSNev]

The way this config works is that it "trys" to obey your request, but if the runtime is unable to "add" any events (excluding the disabled ones) then it will turn around and add them anyway -- so that it's always got at least 1 event.

ie. you can't "disable" all of them.

Which version of the SDK? A quick look at the code appears that it should still be working, but I'm not finding any specific tests for this config value.

I'm going to tag this issue so that at the very least we need to add unit tests around this.

[alfeg]

but if the runtime is unable to "add" any events (excluding the disabled ones) then it will turn around and add them anyway

Is there a way to identify why other events were not able to be added?

I'm using latest Chrome and adding disablePageUnloadEvents: ["unload"] don't produce any changes to Lighthouse report.

Using

Chrome: Version 114.0.5735.199 (Official Build) (64-bit)
@microsoft/applicationinsights-web: 3.0.2

[MSNev]

Is there a way to identify why other events were not able to be added?

No, not without debugging into it, but based on this being Chrome 114, your running in a browser environment that should be handling this. Basically, it tracks whether the addEventHandler could be called for either the window, body or document for each event. If at least 1 can be added then it "obeys" the exclude request....

[MSNev]

Or at least it should be @Karlie-777, @siyuniu-ms do you have some cycles to investigate this?

[Karlie-777]

@alfeg so the variable passed in addHousekeepingBeforeUnload(appInsightsInstance: IApplicationInsights) is the aisku (web) instance but not applicationInsightsCore instance. And here is the eventListener and it is by design that at least one event is added. But when you try set disablePageUnloadEvents: true, could you see event handlers are still added?

[alfeg]

@Karlie-777

Also can confirm issue - there is no config provider is missing disablePageUnloadEvents property at all, sot it's no matter what we set there

And I guess I found issue in debug and were able to fix by patching node_modules files in https://github.com/microsoft/ApplicationInsights-JS/blob/b89849b7c2619a233a1c6fa58bf840e967de99b1/AISKU/src/AISku.ts#L332C1-L333C60

Naive fix is to read config value not from analyticsPlugin but from appInsightsInstance directly

let analyticsPlugin = appInsightsInstance.appInsights;
- let theConfig = analyticsPlugin.config;
+ let theConfig = appInsightsInstance.config;

But it seems to me that actual fix is a bit more trickier because appInsightsInstance do not contain config per TS declarations

[MSNev]

Hmm, it "should" contain the config, but its now a getter property and not just a standard property.

But looking at the code this is returning the "extension" config only now and the disablePageUnloadEvents is actually defined on the "core" IConfiguration.... So this is a side-effect of the new "shared" configuration and a previous "bug" cause by use "supporting" setting the configuration on either the main config or within the "extensionConfig.XXXX". While we still "support" this behavor we achieve this by copying the value from the root to the extensionConfig version (and making it reactive) during initialization -- but this value is not "defined" so that doesn't happen.

As a "temporary" workaround you could try adding to both (as we need to fix this addHouseKeepingCode to use the code instance) so that temporarily at lease it should be present

[alfeg]

@MSNev Thanks for response

But I'm not sure how to implement workaround.
app.appInsights.config is null before app.loadAppInsights is executed. And I cannot set there anything, because app.appInsights.config is readonly property.

Guess I just need to wait for a new release

[MSNev]

Yeh, it will be null before the sdk is initialized (via the loadAppInsights), what i mentioned above is that you would need to include it in the configuration passed to the loadAppInsights), something like

{ 
    connectionString: "",
    disablePageUnloadEvents : [ ... ],
    extensionConfig: {
        ApplicationInsightsAnalytics: {
            disablePageUnloadEvents : [ ... ],
        }
    }
}

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.