BrowserOptions needs to support the TransportOptions generic.

[HaoZhouInRC]

Is there an existing issue for this?

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

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/react

SDK Version

8.27.0

Framework Version

react 16

Link to Sentry event

No response

Reproduction Example/SDK Setup

typescript version: 4.6.2=

  Sentry.init({
    transportOptions: { shouldStore: () => true },
  });

Steps to Reproduce

  Sentry.init({
    transportOptions: { shouldStore: () => true },
  });

Expected Result

No TypeScript errors.

Actual Result

TS2322: Type
{   shouldStore: (envelope: Envelope) => boolean; }
is not assignable to type Partial<BrowserTransportOptions>
Object literal may only specify known properties, and shouldStore does not exist in type Partial<BrowserTransportOptions>
options.d.ts(64, 5): The expected type comes from property transportOptions which is declared here on type BrowserOptions

[s1gr1d]

Thanks for reporting, I will have a look into this.

[s1gr1d]

Did you add transport: Sentry.makeBrowserOfflineTransport(Sentry.makeFetchTransport) in your Sentry.init call (like here)?

Only when this is added, the additional options can be passed to transportOptions and TypeScript cannot infer the types here (as it does not know if you added makeBrowserOfflineTransport or not.

@timfish Is there a specific reason the types are added to transportOptions only? Would it be possible to add a second argument options to makeBrowserOfflineTransport that makes it possible to type-safely add the options.

[timfish]

Yep, the transportOptions type is supplemented by whichever transport you have set so you also need to set the transport.

import * as Sentry from "@sentry/browser";

Sentry.init({
  dsn: "https://[email protected]/0",
  transport: Sentry.makeBrowserOfflineTransport(Sentry.makeFetchTransport)
  transportOptions: {
    // 
  }
})

Is there a specific reason the types are added to transportOptions only?

I might be wrong but I think passing the config like this was an API design choice long before the offline support was added.

Maybe @AbhiPrasad can clarify?

Would it be possible to add a second argument options to makeBrowserOfflineTransport that makes it possible to type-safely add the options

That would be possible but we'd have to decide how we'd cater for merging settings from the two sources. Which would have priority, etc.

[AbhiPrasad]

I might be wrong but I think passing the config like this was an API design choice long before the offline support was added.

It was done before my time, so we kept the same API for backwards compatibilities sake back when we worked on 7.x. Then with 8.x we didn't change anything.

Options does take a transport options as a generic: Options<BrowserTransportOptions>, so casting the type should work to make it typesafe.

as Options<OfflineTransportOptions & BrowserTransportOptions>

[mydea]

IMHO the typecast strategy is not really ideal here, especially since e.g. Options etc. are not exported from the packages, so you need to install @sentry/types to get this to work etc... 🤔

I can confirm that this:

Sentry.init({
  transport: Sentry.makeBrowserOfflineTransport(Sentry.makeFetchTransport),
  transportOptions: {
    flushAtStartup: true,
  },
});

does not work, so the inferral of the transport type does not seem to work - @timfish could you take a look at this maybe?

[timfish]

Since we're all mostly agreed that the separate transportOptions is a bit strange, we could deprecate transportOptions and move to passing options directly to the transports?

The only downside I can see here is that if you want to pass options to the default transport, you'll need to pass the transport too. We could start exporting makeDefaultTransport from each SDK so users don't need to specifically know the name/type of the default transport.

So this:

import * as Sentry from '@sentry/browser';

Sentry.init({
  dsn: '__DSN__',
  transportOptions: {
    headers: { something: 'some-header' }
  }
});

Would become:

import * as Sentry from '@sentry/browser';

Sentry.init({
  dsn: '__DSN__',
  transport: Sentry.makeDefaultTransport({
    headers: { something: 'some-header' }
  })
});

[AbhiPrasad]

We spoke as a team and decided that transportOptions going away is a good thing, let's favour the explicit API of using transport and passing in options there.

for now we can deprecate and change accordingly.

[timfish]

Unfortunately, I looks like we won't be able to keep a single makeXTransport export and be able to pass it both with and without calling the function. I've tried some type shenanigans and it doesn't look like even the latest TypeScript can infer the correct types when the options are declared separately.

We can either leave this change until the next major or we'll need to create new transports that always need to be called as a function. We could add new transports which don't start with make* and the deprecate the make* ones?

Now:

import * as Sentry from '@sentry/browser';

Sentry.init({
  dsn: '__DSN__',
  transport: Sentry.makeFetchTransport
});

After:

import * as Sentry from '@sentry/browser';

Sentry.init({
  dsn: '__DSN__',
  transport: Sentry.fetchTransport()
});

Would depreciation of all the existing transports be considered too annoying for customers? How many users actually ref. the existing transports?

[AbhiPrasad]

Let's do this in v9 - ref https://github.com/getsentry/sentry-javascript/issues/14149

[mydea]

I guess we did not do this in v9 😅 @timfish do you have thoughts on how we could possibly "fix" this in a backwards-compatible way 😅

[timfish]

From my testing, the only way to fix this in a backwards compatible way is to use new names for the function transports and deprecate the old exports.

[qmg-fmichalski]

To anyone struggling with this one, I am simply type-casting to add the support for OfflineTransportOptions

      transportOptions: <Partial<BrowserTransportOptions & OfflineTransportOptions>>{ maxQueueSize: 20 },