feat(core): Add type & utility for function-based integrations #9818

mydea · 2023-12-13T12:50:10Z

This PR adds new types for function-based integrations, that eventually (in v8) should fully replace the class-based functions.

This also introduces a small helper function to make writing such integrations easier (as we need to set an id/name etc. on the integration). With this, you can write an integration like this:

const inboundFiltersIntegration = makeIntegrationFn(
  'InboundFilters', 
  (options: Partial<InboundFiltersOptions>) => {
  return {
    processEvent(event, _hint, client) {
      const clientOptions = client.getOptions();
      const mergedOptions = _mergeOptions(options, clientOptions);
      return _shouldDropEvent(event, mergedOptions) ? null : event;
    }
  }
});

And you get a fully typed integration ready to go!

For backwards compatibility, and so that we can actually start converting integrations in v7 already, this PR also adds a small utility convertIntegrationFnToClass() to convert such an integration to the "current" integration class syntax.

So we can actually already start porting integrations over like this:

/** Inbound filters configurable by the user */
// eslint-disable-next-line deprecation/deprecation
export const InboundFilters = convertIntegrationFnToClass(inboundFiltersIntegration);

Then, in v8 we only have to remove all the convertIntegrationFnToClass calls, export the integration functions directly, and update the overall integration types which can be passed to init() etc.

github-actions · 2023-12-13T12:59:01Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	75.24 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	66.61 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	60.21 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.29 KB (-0.02% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	29.89 KB (-0.05% 🔽)
@sentry/browser - Webpack (gzipped)	21.56 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	72.63 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	64.34 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	30.6 KB (+0.1% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	22.66 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	202.52 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	92.65 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	67.65 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	33.52 KB (+0.15% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	66.96 KB (-0.02% 🔽)
@sentry/react - Webpack (gzipped)	21.59 KB (-0.06% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	83.76 KB (+0.06% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	48.42 KB (+0.05% 🔺)
@sentry-internal/feedback - Webpack (gzipped)	16.19 KB (0%)

mydea · 2023-12-13T13:04:12Z

packages/core/src/integrations/inboundfilters.ts

-   */
-  public setupOnce(_addGlobalEventProcessor: unknown, _getCurrentHub: unknown): void {
-    // noop
+const inboundFiltersIntegration = makeIntegrationFn('InboundFilters', (options: Partial<InboundFiltersOptions>) => {


I just converted a single integration for now to show this in "reality", but I can also extract this out into a follow up PR.

Let's just keep it here, the change is pretty small.

Lms24

I'm not sure if I'm missing something here but I'm thinking about the defineConfig patterns from Vite, Astro, Nuxt, etc and I'm wondering if we could simplify this helper function a little in a similar manner; or actually even more than that:

const inboundFilters =
  (options: Partial<InboundFiltersOptions>) => {
  // do whatever you need to do here e.g. assign default values
  return {
    name: 'InboundFilters',
    processEvent(event, _hint, client) {
      const clientOptions = client.getOptions();
      const mergedOptions = _mergeOptions(options, clientOptions);
      return _shouldDropEvent(event, mergedOptions) ? null : event;
    }
  }
});

I get that this simplification doesn't allow for name checks before initalization but I'm wondering if we even need that. Usually, instrumentation should happen in setup(once) anyway.

anonrig · 2023-12-13T13:44:51Z

packages/core/src/index.ts

+export {
+  getIntegrationsToSetup,
+  addIntegration,
+  // eslint-disable-next-line deprecation/deprecation


If we are using deprecated functions, I think we should disable this rule instead of overriding it everywhere

The idea here was to make it easier to spot that this should be removed in v8 🤔 no strong feelings, I just wanted to make it clear that this is a temporary thing that will be removed in v8 again - generally, we should avoid using deprecated stuff ourselves, so I think the rule is good.

packages/core/src/integration.ts

anonrig · 2023-12-13T13:47:35Z

packages/core/src/integrations/inboundfilters.ts

@@ -84,8 +64,7 @@ export function _mergeOptions(
  };
 }

-/** JSDoc */
-export function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
+function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {


Why does this function start with underscore?

they are "private" - we are not super consistent there, sometimes we prefix them and sometimes not. but specifically here I did not change that, just removed the export as we don't actually use it anywhere!

mydea · 2023-12-13T13:59:33Z

I'm not sure if I'm missing something here but I'm thinking about the defineConfig patterns from Vite, Astro, Nuxt, etc and I'm wondering if we could simplify this helper function a little in a similar manner; or actually even more than that:
const inboundFilters =
  (options: Partial<InboundFiltersOptions>) => {
  // do whatever you need to do here e.g. assign default values
  return {
    name: 'InboundFilters',
    processEvent(event, _hint, client) {
      const clientOptions = client.getOptions();
      const mergedOptions = _mergeOptions(options, clientOptions);
      return _shouldDropEvent(event, mergedOptions) ? null : event;
    }
  }
});
I get that this simplification doesn't allow for name checks before initalization but I'm wondering if we even need that. Usually, instrumentation should happen in setup(once) anyway.

Hmm, can we get rid of the id? If so, then yeah we don't really need this - this is mostly to make this id/name stuff easier to handle. 🤔

AbhiPrasad

Hmm, can we get rid of the id

I think we can because id is basically equivalent to name everywhere.

AbhiPrasad · 2023-12-14T13:40:38Z

packages/core/src/integrations/inboundfilters.ts

-   */
-  public setupOnce(_addGlobalEventProcessor: unknown, _getCurrentHub: unknown): void {
-    // noop
+const inboundFiltersIntegration = makeIntegrationFn('InboundFilters', (options: Partial<InboundFiltersOptions>) => {


Let's just keep it here, the change is pretty small.

mydea · 2023-12-14T14:06:01Z

Hmm, if we get rid of the id, do we still want to have the util function? It doesn't really do anything, but makes typing much nicer...

Function:

function makeIntegrationFn<Fn extends IntegrationFn>(fn: Fn): Fn {
  return fn;
}

But using it allows to omit all types:

const inboundFiltersIntegration = makeIntegrationFn((options: Partial<InboundFiltersOptions>) => {
  return {
    name: INTEGRATION_NAME,
    // look mom, no types!
    processEvent(event, _hint, client) {
      const clientOptions = client.getOptions();
      const mergedOptions = _mergeOptions(options, clientOptions);
      return _shouldDropEvent(event, mergedOptions) ? null : event;
    },
  };
});

is this worth it??

lforst · 2023-12-15T10:52:36Z

packages/types/src/integration.ts

+export type IntegrationFn<Fn extends (...rest: any[]) => IntegrationFnResult> = { id: string } & Fn;
+
+export interface IntegrationFnResult {


I don't think these are necessary or a good idea. I would prefer if we just have an Integration interface. These types seem overkill and don't add much.

The thing is that for now these are not the same, as setupOnce is optional here. In v8 we can get rid of this and have just a single Integration interface again!

lforst · 2023-12-15T10:54:35Z

packages/core/src/integration.ts

+ * This will ensure to add the given name both to the function definition (as id),
+ * as well as to the integration return value.
+ */
+export function makeIntegrationFn<


This function is imo not really doing anything. I think we (and eventually our users) are just better off defining integrations as follows.

export function MyCustomIntegration(): Integration {};

Easy, simple, well-typed. The rollup and Vite ecosystem is proof that this pattern works and is well understood.

Will remove this!

This reverts commit 31887622b297c094e37ccfa00b2a8708bd0d73c7.

mydea · 2023-12-15T11:23:03Z

Updated this to remove the makeIntergrationFn util as we don't need that anymore. Now it's just the new types (to make setupOnce optional there), as well as the util to convert to a class for v7.

mydea requested review from anonrig, lforst, Lms24 and AbhiPrasad December 13, 2023 12:50

mydea self-assigned this Dec 13, 2023

mydea mentioned this pull request Dec 13, 2023

ref(deno): Update deno integrations to avoid setupOnce #9812

Merged

mydea commented Dec 13, 2023

View reviewed changes

Lms24 reviewed Dec 13, 2023

View reviewed changes

anonrig reviewed Dec 13, 2023

View reviewed changes

AbhiPrasad approved these changes Dec 14, 2023

View reviewed changes

lforst reviewed Dec 15, 2023

View reviewed changes

mydea added 10 commits December 15, 2023 12:18

WIP: Add makeIntegrationFn util

d53b30c

WIP: add convertIntegrationFnToClass util

ed7652c

export it

a91624a

refactor one integration as an example

a12c405

small doc

67e2daa

fix linting

57ed4d2

PR feedback

b1a536d

try make it preprocess??

a053678

Revert "try make it preprocess??"

8771e99

This reverts commit 31887622b297c094e37ccfa00b2a8708bd0d73c7.

reduce

c5c8d82

mydea changed the title ~~feat(core): Add utilities for function-based integrations~~ feat(core): Add type & utility for function-based integrations Dec 15, 2023

mydea force-pushed the fn/integration-fn branch from 4db0b7a to 2a64201 Compare December 15, 2023 11:21

avoid unncessary function

87446e9

mydea force-pushed the fn/integration-fn branch from 2a64201 to 87446e9 Compare December 15, 2023 11:22

fix linting

a2901ce

AbhiPrasad approved these changes Dec 15, 2023

View reviewed changes

mydea merged commit 01a4cc9 into develop Dec 18, 2023

mydea deleted the fn/integration-fn branch December 18, 2023 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add type & utility for function-based integrations #9818

feat(core): Add type & utility for function-based integrations #9818

mydea commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 •

edited

Loading

mydea Dec 13, 2023

AbhiPrasad Dec 14, 2023

Lms24 left a comment •

edited

Loading

anonrig Dec 13, 2023

mydea Dec 13, 2023

anonrig Dec 13, 2023

mydea Dec 13, 2023

mydea commented Dec 13, 2023

AbhiPrasad left a comment

AbhiPrasad Dec 14, 2023

mydea commented Dec 14, 2023

lforst Dec 15, 2023

mydea Dec 15, 2023

lforst Dec 15, 2023

mydea Dec 15, 2023 •

edited

Loading

mydea commented Dec 15, 2023

		export type IntegrationFn<Fn extends (...rest: any[]) => IntegrationFnResult> = { id: string } & Fn;

		export interface IntegrationFnResult {

feat(core): Add type & utility for function-based integrations #9818

feat(core): Add type & utility for function-based integrations #9818

Conversation

mydea commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Dec 13, 2023

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

mydea commented Dec 15, 2023

github-actions bot commented Dec 13, 2023 •

edited

Loading

Lms24 left a comment •

edited

Loading

mydea Dec 15, 2023 •

edited

Loading