fix(instrumentation-browser-navigation): use 'declare' to avoid JS class field initialization surprise (open-telemetry#3326)

Before this change these class fields were being set to undefined after the
super() call, which blows away values set in the .enable() called by the
super constructor.

This also reverts the following change that was attempting to work around test failures:
   "fix(instrumentation-browser-navigation): improve test stability with … (open-telemetry#3323)"

This also adds a guard on .enable() being called multiple times because it
isn't idempotent.

Fixes: open-telemetry#3310

Author: trentm

packages/instrumentation-browser-navigation/src/instrumentation.ts

@@ -40,13 +40,18 @@ export const ATTR_BROWSER_NAVIGATION_TYPE = 'browser.navigation.type';
 export class BrowserNavigationInstrumentation extends InstrumentationBase<BrowserNavigationInstrumentationConfig> {
   applyCustomLogRecordData: ApplyCustomLogRecordDataFunction | undefined =
     undefined;
-  sanitizeUrl?: SanitizeUrlFunction; // No default sanitization
-  private _onLoadHandler?: () => void;
-  private _onPopStateHandler?: (ev: PopStateEvent) => void;
-  private _onNavigateHandler?: (ev: Event) => void;
+  declare sanitizeUrl?: SanitizeUrlFunction; // No default sanitization
+  declare private _onLoadHandler?: () => void;
+  declare private _onPopStateHandler?: (ev: PopStateEvent) => void;
+  declare private _onNavigateHandler?: (ev: Event) => void;
   private _lastUrl: string = location.href;
   private _hasProcessedInitialLoad: boolean = false;
 
+  // Note: Intentionally *not* using `_enabled` as the field name to avoid
+  // any possible confusion with the `_enabled` field used on the *Node.js*
+  // InstrumentationBase class.
+  declare private _isEnabled: boolean;
+
   /**
    *
    * @param config
@@ -133,8 +138,7 @@ export class BrowserNavigationInstrumentation extends InstrumentationBase<Browse
     // Check if document has already loaded completely
     if (document.readyState === 'complete' && !this._hasProcessedInitialLoad) {
       this._hasProcessedInitialLoad = true;
-      // Use setTimeout to allow tests to reset exporter before this fires
-      setTimeout(() => this._onHardNavigation(), 0);
+      this._onHardNavigation();
       return;
     }
 
@@ -155,6 +159,11 @@ export class BrowserNavigationInstrumentation extends InstrumentationBase<Browse
    * implements enable function
    */
   override enable() {
+    if (this._isEnabled) {
+      return;
+    }
+    this._isEnabled = true;
+
     const cfg = this.getConfig() as BrowserNavigationInstrumentationConfig;
     const useNavigationApiIfAvailable = !!cfg.useNavigationApiIfAvailable;
     const navigationApi =
@@ -200,6 +209,11 @@ export class BrowserNavigationInstrumentation extends InstrumentationBase<Browse
    * implements disable function
    */
   override disable() {
+    if (!this._isEnabled) {
+      return;
+    }
+    this._isEnabled = false;
+
     this._unpatchHistoryApi();
     if (this._onLoadHandler) {
       document.removeEventListener('DOMContentLoaded', this._onLoadHandler);
@@ -243,8 +257,10 @@ export class BrowserNavigationInstrumentation extends InstrumentationBase<Browse
   }
 
   private _patchHistoryApi(): void {
-    // unpatching here disables other instrumentation that use the same api to wrap history, commenting it out
-    // this._unpatchHistoryApi();
+    // Unpatching here disables other instrumentation that use the same api to
+    // wrap history, commenting it out.
+    //    this._unpatchHistoryApi();
+    // TODO: For example, instrumentation-user-interaction also patches `history`. Do the two instrumentations work at the same time?
     this._wrap(
       history,
       'replaceState',