-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
-
getGlobalObject seems to be overused throughout the SDKs. Why not use window in @sentry/browser and global in @sentry/node? What context/knowledge am I missing?
getGlobalObject was clearly designed to help get the global object when the code might be run on different runtimes (ie. frontend and backend) but those usages are minimal.
In many cases, it appears to be used to help extend the global type like this:
sentry-javascript/packages/nextjs/src/performance/client.ts
Lines 14 to 20 in 903addf
But you can extend the global via declare global so the types work and simply use window:
declare global { const __BUILD_MANIFEST: { sortedPages?: string[] } | undefined; } const pageRoutes = (window.__BUILD_MANIFEST || {}).sortedPages;
Do we avoid using declare global for the below global properties because this leaks into the global type for consumers of the SDKs?
sentry-javascript/packages/utils/src/global.ts
Lines 13 to 27 in 903addf
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 1 comment
-
As per a @kamilogorek knowledge bomb on slack:
getGlobalObjectseems to be overused throughout the SDKs. Why not use window in@sentry/browserand global in@sentry/node? What context/knowledge am I missing?
- There is no
windowinWebWorker - There used to be no
windowinjest/phantomjs - There is no
windowin some obscure "compile-to" native environments - Also some of the code that uses
getGlobalObjectis shared across all SDKs, so we cannot choose for browser/node, the util has to do it - eg.
getMainCarrierinhubuses it, and its shared for node/browser - And our main
loggeruses it as well - So effectively it has to be there. And if its there, why not use it everywhere instead of sometimes
globalin node, sometimeswindowin browser, sometimesgetGlobalObject. Unification here is better, as you can easily grep for global API usages - It wont make bundle larger, as its there. And as for performance, its a single op for existence check, 0 overhead basically
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1