Sentry typescript type error introduced in 7.16.0

[nahidakbar]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/serverless

SDK Version

7.16.0

Framework Version

Serverless

Link to Sentry event

No response

Steps to Reproduce

For my GCP serverless codebase,

Nodejs v16.13.2

Typescript 4.8.3

if I upgrade my sentry packages from 7.15.0 to 7.16.0, it gives error

Expected Result

No error

Actual Result

node_modules/@sentry/utils/types/browser.d.ts:4:69 - error TS2304: Cannot find name 'Window'.

[AbhiPrasad]

This is occuring because we have top level casting for Window, where previously it was gated behind a function call.

@timfish could we move these exports into the browser package to fix this?

[AbhiPrasad]

ref #5831

[timfish]

WINDOW is still used extensively in utils so moving it from there is not an option for now.

I guess the only option is to put it behind a function call for now?

export function WINDOW(): Window & InternalGlobal {
  return GLOBAL_OBJ as Window & InternalGlobal;
}

[AbhiPrasad]

We'll probably have to cast at call site because top level call will still break TS definitions.

Why can't we move the WINDOW functions to @sentry/browser? The only file that is maybe a little tricky to do is time.ts, but that can be split up.

[timfish]

WINDOW is currently used in:

• browser.ts - getLocationHref and getDomElement could probably be moved to @sentry/browser
• supports.ts - Much of this code can be moved to @sentry/browser too
• instrument.ts - addInstrumentationHandler is used in @sentry/tracing. Not sure how to solve that.
• time.ts - Tricky to split up since this uses perf_hooks on node.js. performance should almost certainly be passed down from the client like TextEncoder.

Earlier we had this code in utils with no issues:

sentry-javascript/packages/utils/src/global.ts

Lines 36 to 46 in 4cc07f6

	export function getGlobalObject<T>(): T & SentryGlobal {
	return (
	isNodeEnv()
	? global
	: typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
	? window // eslint-disable-line no-restricted-globals
	: typeof self !== 'undefined'
	? self
	: fallbackGlobalObject
	) as T & SentryGlobal;
	}

sentry-javascript/packages/utils/src/browser.ts

Line 118 in 4cc07f6

const global = getGlobalObject<Window>();

So Window WAS passed from the call site, but in many cases, the call site was still in utils!

[AbhiPrasad]

Yeah instrument.ts and time.ts make this tricky. time.ts we maybe can just solve by just having a node specific time.ts and browser specific time.ts, and not bothering to dependency inject or keep a agnostic file, but instrument prob cannot be solved that way (unless we also inject the global maybe?).

So Window WAS passed from the call site, but in many cases, the call site was still in utils!

I think the issue here is that a top level def of export function WINDOW(): Window & InternalGlobal { will break. We'll maybe have to bring back getGlobalObject just for these functions.

[timfish]

We'll maybe have to bring back getGlobalObject just for these functions.

Yeah, it doesn't need to be exported and can be used only internally in utils

[timfish]

it doesn't need to be exported and can be used only internally in utils

Oh, that is not the case since we can't keep WINDOW in utils.

Do you think a generic will work with a default type?

export function WINDOW<T = Window>(): InternalGlobal & T {
  return GLOBAL_OBJ as InternalGlobal & T;
}

[AbhiPrasad]

Do you think a generic will work with a default type?

I don't think so.

Oh, that is not the case since we can't keep WINDOW in utils.

We can still keep it as long as it's within a function call right? We don't have to use the default generic, just leave it as T, same as getGlobalObject.