Uh oh!
There was an error while loading. Please reload this page.
feat(core): Add type & utility for function-based integrations - #9818
Conversation
size-limit report 📦
|
| */ | ||
| public setupOnce(_addGlobalEventProcessor: unknown, _getCurrentHub: unknown): void { | ||
| // noop | ||
| const inboundFiltersIntegration = makeIntegrationFn('InboundFilters', (options: Partial<InboundFiltersOptions>) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's just keep it here, the change is pretty small.
There was a problem hiding this comment.
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:
constinboundFilters=(options: Partial<InboundFiltersOptions>)=>{// do whatever you need to do here e.g. assign default valuesreturn{name: 'InboundFilters',processEvent(event,_hint,client){constclientOptions=client.getOptions();constmergedOptions=_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.
| export { | ||
| getIntegrationsToSetup, | ||
| addIntegration, | ||
| // eslint-disable-next-line deprecation/deprecation |
There was a problem hiding this comment.
If we are using deprecated functions, I think we should disable this rule instead of overriding it everywhere
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /** JSDoc */ | ||
| export function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean { | ||
| function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean { |
There was a problem hiding this comment.
Why does this function start with underscore?
There was a problem hiding this comment.
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
commented
Dec 13, 2023
Hmm, can we get rid of the |
AbhiPrasad
left a comment
There was a problem hiding this comment.
Hmm, can we get rid of the id
I think we can because id is basically equivalent to name everywhere.
| */ | ||
| public setupOnce(_addGlobalEventProcessor: unknown, _getCurrentHub: unknown): void { | ||
| // noop | ||
| const inboundFiltersIntegration = makeIntegrationFn('InboundFilters', (options: Partial<InboundFiltersOptions>) => { |
There was a problem hiding this comment.
Let's just keep it here, the change is pretty small.
mydea
commented
Dec 14, 2023
Hmm, if we get rid of the Function: functionmakeIntegrationFn<FnextendsIntegrationFn>(fn: Fn): Fn {returnfn;}But using it allows to omit all types: constinboundFiltersIntegration=makeIntegrationFn((options: Partial<InboundFiltersOptions>)=>{return{name: INTEGRATION_NAME,// look mom, no types!processEvent(event,_hint,client){constclientOptions=client.getOptions();constmergedOptions=_mergeOptions(options,clientOptions);return_shouldDropEvent(event,mergedOptions) ? null : event;},};});is this worth it?? |
| export type IntegrationFn<Fn extends (...rest: any[]) => IntegrationFnResult> = { id: string } & Fn; | ||
| export interface IntegrationFnResult { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| * 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< |
There was a problem hiding this comment.
This function is imo not really doing anything. I think we (and eventually our users) are just better off defining integrations as follows.
exportfunctionMyCustomIntegration(): Integration{};Easy, simple, well-typed. The rollup and Vite ecosystem is proof that this pattern works and is well understood.
This reverts commit 31887622b297c094e37ccfa00b2a8708bd0d73c7.
4db0b7a to
2a64201Compare2a64201 to
87446e9Comparemydea
commented
Dec 15, 2023
Updated this to remove the |
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:
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:
Then, in v8 we only have to remove all the
convertIntegrationFnToClasscalls, export the integration functions directly, and update the overall integration types which can be passed toinit()etc.