fix(cloudflare)!: add better type handling for additional context

[chanceaclark]

What/Why?

Adds better handling of Cloudflare context for types. Using any and unknown in an interface was causing type mismatch, so even though it's more verbose, we should match the types needed from Cloudflare:

Noticed a v9 is coming soon, so thought it's a good opportunity to add a BC now.

BREAKING CHANGE:
Updates the cloudflare withSentry handler generic type to allow passing in additional types used by ExportedHandler.

Before:

Sentry.withSentry<ExportedHandler<Env>>(...)

After:

Sentry.withSentry<Env>(...)

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

[Lms24]

Hi @chanceaclark thanks for opening this PR! Would you mind sharing a reproducible example where you get a type error with the currently typed withSentry function? Ideally, we find a way to avoid the breaking change because our usual process around breaking changes is to deprecate an existing API for a while before we introduce the breaking change.

If we can understand the concrete use case better, we can make a better decision how to proceed with this PR.

Maybe a way forward is also to export a new API with the adjusted types if we can't avoid breaking. We'll see.

[Lms24]

By the way, we have an e2e test application in the repo. Feel free to add another handler that would throw the type error with the current type definition. This can help us understand the problem and at the same time adds a regression test, thanks :)

[chanceaclark]

Hey @Lms24 sorry to get back to you so late. Added a test case in the package and made a sample repo with the type error I was running into:

Test case: develop...chanceaclark:sentry-javascript:test/cf-additional-types
Repo: https://github.com/chanceaclark/dispatch-worker-sentry

Similarly it happens even when using the generic instead of the satisfies constraint.

I also tried adjusting the sentry lib types to not make it a breaking change but the same issue was occuring, for example these:

export declare function withSentry<E extends ExportedHandler<unknown, unknown, unknown>>(optionsCallback: (env: ExtractEnv<E>) => CloudflareOptions, handler: E): E;
// or
export declare function withSentry<E extends ExportedHandler<any, any, any>>(optionsCallback: (env: ExtractEnv<E>) => CloudflareOptions, handler: E): E;
// or
export declare function withSentry<E extends ExportedHandler>(optionsCallback: (env: ExtractEnv<E>) => CloudflareOptions, handler: E): E;

The only thing I found that seems to work is setting the generic with a default type, but that doesn't constrain the type which I don't think we want. I guess my solution in the PR doesn't constrain the type either though 😅

export declare function withSentry<E = ExportedHandler>(optionsCallback: (env: ExtractEnv<E>) => CloudflareOptions, handler: E): E;

[CanRau]

Found this as I'm also getting type mismatch errors in our monorepo, unfortunately can't share a repro right now tho 😇

I'm getting Type 'unknown' is not assignable to type 'TQueueMessage'.ts(2344)
Where TQueueMessage is our app's definition of queue messages

On first glance the proposed PR looks promising 🤓

[chanceaclark]

I'm tempted to go with the follow so it's not a breaking change:

export declare function withSentry<E = ExportedHandler>(optionsCallback: (env: ExtractEnv<E>) => CloudflareOptions, handler: E): E;

@Lms24 is this something you'd be comfortable with too?

[Lms24]

@chanceaclark sorry for the late reply (I've been out for a couple of days). This sounds reasonable to me but I'll leave this up to @AbhiPrasad to give the final okay. Feel free to make the change though as we won't merge in a breaking change anytime soon.