Should use 'globalThis' instead of 'window' for the browser usage

[cristianmadularu]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

I noticed that when running in browser, I was getting errors when trying to log from a web worker. Upon debugging, I realized that the OTLPExporterBrowserBase is referencing the 'window' instead of using 'globalThis'. As expected, the 'window' is not available in the web worker. I have created a local patch for our project (replacing the usage of 'window' with 'globalThis') but I think that this change should be applied to the published library as well.

I used patch-package to patch @opentelemetry/[email protected] for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/@opentelemetry/otlp-exporter-base/build/esm/platform/browser/OTLPExporterBrowserBase.js b/node_modules/@opentelemetry/otlp-exporter-base/build/esm/platform/browser/OTLPExporterBrowserBase.js
index d763ecb..15d099c 100644
--- a/node_modules/@opentelemetry/otlp-exporter-base/build/esm/platform/browser/OTLPExporterBrowserBase.js
+++ b/node_modules/@opentelemetry/otlp-exporter-base/build/esm/platform/browser/OTLPExporterBrowserBase.js
@@ -42,10 +42,10 @@ var OTLPExporterBrowserBase = /** @class */ (function (_super) {
         return _this;
     }
     OTLPExporterBrowserBase.prototype.onInit = function () {
-        window.addEventListener('unload', this.shutdown);
+        globalThis.addEventListener('unload', this.shutdown);
     };
     OTLPExporterBrowserBase.prototype.onShutdown = function () {
-        window.removeEventListener('unload', this.shutdown);
+        globalThis.removeEventListener('unload', this.shutdown);
     };
     OTLPExporterBrowserBase.prototype.send = function (items, onSuccess, onError) {
         var _this = this;
diff --git a/node_modules/@opentelemetry/otlp-exporter-base/build/esnext/platform/browser/OTLPExporterBrowserBase.js b/node_modules/@opentelemetry/otlp-exporter-base/build/esnext/platform/browser/OTLPExporterBrowserBase.js
index 25f943c..5bf9064 100644
--- a/node_modules/@opentelemetry/otlp-exporter-base/build/esnext/platform/browser/OTLPExporterBrowserBase.js
+++ b/node_modules/@opentelemetry/otlp-exporter-base/build/esnext/platform/browser/OTLPExporterBrowserBase.js
@@ -38,10 +38,10 @@ export class OTLPExporterBrowserBase extends OTLPExporterBase {
         }
     }
     onInit() {
-        window.addEventListener('unload', this.shutdown);
+        globalThis.addEventListener('unload', this.shutdown);
     }
     onShutdown() {
-        window.removeEventListener('unload', this.shutdown);
+        globalThis.removeEventListener('unload', this.shutdown);
     }
     send(items, onSuccess, onError) {
         if (this._shutdownOnce.isCalled) {
diff --git a/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/OTLPExporterBrowserBase.js b/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/OTLPExporterBrowserBase.js
index 1ab9ac7..131c1f6 100644
--- a/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/OTLPExporterBrowserBase.js
+++ b/node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/OTLPExporterBrowserBase.js
@@ -41,10 +41,10 @@ class OTLPExporterBrowserBase extends OTLPExporterBase_1.OTLPExporterBase {
         }
     }
     onInit() {
-        window.addEventListener('unload', this.shutdown);
+        globalThis.addEventListener('unload', this.shutdown);
     }
     onShutdown() {
-        window.removeEventListener('unload', this.shutdown);
+        globalThis.removeEventListener('unload', this.shutdown);
     }
     send(items, onSuccess, onError) {
         if (this._shutdownOnce.isCalled) {

This issue body was partially generated by patch-package.

[legendecas]

I think it would make sense to update the use sites of window in the @opentelemetry/otlp-exporter-base and other packages to use the "global this" variable defined in https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/src/platform/browser/globalThis.ts instead.

[cristianmadularu]

These days, all browsers support 'globalThis' nativelly.
https://caniuse.com/?search=globalThis

I believe that the 'globalThis' defined in https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/src/platform/browser/globalThis.ts was added in order to offer some backwards compatibility with legacy browsers.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[cristianmadularu]

Hi guys, any update on this? I am currently being forced to keep a patched version in order to be able to use it from a web worker.

Thank you

[pichlermarc]

@cristianmadularu, no movement right now; marking this up-for-grabs in case someone wants to pick it up.

[cristianmadularu]

@pichlermarc I can fix it. Could you please give me access so I can push a fix? (2 lines of code)
Thank you

[pichlermarc]

@pichlermarc I can fix it. Could you please give me access so I can push a fix? (2 lines of code) Thank you

Thanks! The usual flow for this is to open a PR from a fork, which should be possible for every GitHub user.

[cristianmadularu]

OK I created a PR :)
I hope I followed all the rules described in the documentation :) I was not sure if I had to manually update the changelog or that is automatically being updated.

@pichlermarc
#4157

Thanks!

[pichlermarc]

OK I created a PR :) I hope I followed all the rules described in the documentation :) I was not sure if I had to manually update the changelog or that is automatically being updated.

Thanks for taking care of this. It's not workng automatically but I took the liberty to add the entry, I'll merge the PR momentarily. 🙂

[cristianmadularu]

Oh great, thank you! :)