-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Avoid class fields all-together #14887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,18 +10,14 @@ import { getEventSpanOptions } from './helpers'; | |
import type { OnEventTarget } from './types'; | ||
|
||
const supportedVersions = ['>=2.0.0']; | ||
const COMPONENT = '@nestjs/event-emitter'; | ||
|
||
/** | ||
* Custom instrumentation for nestjs event-emitter | ||
* | ||
* This hooks into the `OnEvent` decorator, which is applied on event handlers. | ||
*/ | ||
export class SentryNestEventInstrumentation extends InstrumentationBase { | ||
public static readonly COMPONENT = '@nestjs/event-emitter'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chargome & @andreiborza I do not think these are needed/do anything, looking through the instrumentation base class code 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, not sure why I copied it in the first place 👍 |
||
public static readonly COMMON_ATTRIBUTES = { | ||
component: SentryNestEventInstrumentation.COMPONENT, | ||
}; | ||
|
||
public constructor(config: InstrumentationConfig = {}) { | ||
super('sentry-nestjs-event', SDK_VERSION, config); | ||
} | ||
|
@@ -30,10 +26,7 @@ export class SentryNestEventInstrumentation extends InstrumentationBase { | |
* Initializes the instrumentation by defining the modules to be patched. | ||
*/ | ||
public init(): InstrumentationNodeModuleDefinition { | ||
const moduleDef = new InstrumentationNodeModuleDefinition( | ||
SentryNestEventInstrumentation.COMPONENT, | ||
supportedVersions, | ||
); | ||
const moduleDef = new InstrumentationNodeModuleDefinition(COMPONENT, supportedVersions); | ||
|
||
moduleDef.files.push(this._getOnEventFileInstrumentation(supportedVersions)); | ||
return moduleDef; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Instead of defining these fields in the constructor, can we declare them as regular methods (i.e. without the assignment)? I might be missing something as to why we didn't do this before though
So basically, like the public methods above, just as private? (Which I'm aware doesn't prevent them from being called externally but that should be fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to do
bind
then a bunch of times, which... is also possible probably, let me check the bundle size diff!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote this here to generally be a bit cleaner, so this should make more sense now!