Skip to content

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

Merged
merged 5 commits into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/migration/v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ Sentry.init({

In v9, an `undefined` value will be treated the same as if the value is not defined at all. You'll need to set `tracesSampleRate: 0` if you want to enable tracing without performance.

- The `getCurrentHub().getIntegration(IntegrationClass)` method will always return `null` in v9. This has already stopped working mostly in v8, because we stopped exposing integration classes. In v9, the fallback behavior has been removed. Note that this does not change the type signature and is thus not technically breaking, but still worth pointing out.

### `@sentry/node`

- When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed.
Expand Down Expand Up @@ -208,6 +210,7 @@ This led to some duplication, where we had to keep an interface in `@sentry/type
Since v9, the types have been merged into `@sentry/core`, which removed some of this duplication. This means that certain things that used to be a separate interface, will not expect an actual instance of the class/concrete implementation. This should not affect most users, unless you relied on passing things with a similar shape to internal methods. The following types are affected:

- `Scope` now always expects the `Scope` class
- The `IntegrationClass` type is no longer exported - it was not used anymore. Instead, use `Integration` or `IntegrationFn`.

# No Version Support Timeline

Expand Down
4 changes: 4 additions & 0 deletions packages/angular/.eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ module.exports = {
},
extends: ['../../.eslintrc.js'],
ignorePatterns: ['setup-test.ts', 'patch-vitest.ts'],
rules: {
// Angular transpiles this correctly/relies on this
'@sentry-internal/sdk/no-class-field-initializers': 'off',
},
};
7 changes: 3 additions & 4 deletions packages/core/src/getCurrentHubShim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
setUser,
startSession,
} from './exports';
import type { Client, EventHint, Hub, Integration, IntegrationClass, SeverityLevel } from './types-hoist';
import type { Client, EventHint, Hub, Integration, SeverityLevel } from './types-hoist';

/**
* This is for legacy reasons, and returns a proxy object instead of a hub to be used.
Expand Down Expand Up @@ -48,9 +48,8 @@ export function getCurrentHubShim(): Hub {
setExtras,
setContext,

getIntegration<T extends Integration>(integration: IntegrationClass<T>): T | null {
const client = getClient();
return (client && client.getIntegrationByName<T>(integration.id)) || null;
getIntegration<T extends Integration>(_integration: unknown): T | null {
return null;
},

startSession,
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/types-hoist/hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Breadcrumb, BreadcrumbHint } from './breadcrumb';
import type { Client } from './client';
import type { Event, EventHint } from './event';
import type { Extra, Extras } from './extra';
import type { Integration, IntegrationClass } from './integration';
import type { Integration } from './integration';
import type { Primitive } from './misc';
import type { Session } from './session';
import type { SeverityLevel } from './severity';
Expand Down Expand Up @@ -171,9 +171,9 @@ export interface Hub {
/**
* Returns the integration if installed on the current client.
*
* @deprecated Use `Sentry.getClient().getIntegration()` instead.
* @deprecated Use `Sentry.getClient().getIntegrationByName()` instead.
*/
getIntegration<T extends Integration>(integration: IntegrationClass<T>): T | null;
getIntegration<T extends Integration>(integration: unknown): T | null;

/**
* Starts a new `Session`, sets on the current scope and returns it.
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/types-hoist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export type { Exception } from './exception';
export type { Extra, Extras } from './extra';
// eslint-disable-next-line deprecation/deprecation
export type { Hub } from './hub';
export type { Integration, IntegrationClass, IntegrationFn } from './integration';
export type { Integration, IntegrationFn } from './integration';
export type { Mechanism } from './mechanism';
export type { ExtractedNodeRequestData, HttpHeaderValue, Primitive, WorkerLocation } from './misc';
export type { ClientOptions, Options } from './options';
Expand Down
10 changes: 0 additions & 10 deletions packages/core/src/types-hoist/integration.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,6 @@
import type { Client } from './client';
import type { Event, EventHint } from './event';

/** Integration Class Interface */
export interface IntegrationClass<T> {
/**
* Property that holds the integration name
*/
id: string;

new (...args: any[]): T;
}

/** Integration interface */
export interface Integration {
/**
Expand Down
89 changes: 44 additions & 45 deletions packages/core/src/utils-hoist/syncpromise.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/explicit-function-return-type */
/* eslint-disable @typescript-eslint/no-explicit-any */
import { isThenable } from './is';

Expand Down Expand Up @@ -40,29 +39,25 @@ export function rejectedSyncPromise<T = never>(reason?: any): PromiseLike<T> {
});
}

type Executor<T> = (resolve: (value?: T | PromiseLike<T> | null) => void, reject: (reason?: any) => void) => void;

/**
* Thenable class that behaves like a Promise and follows it's interface
* but is not async internally
*/
class SyncPromise<T> implements PromiseLike<T> {
export class SyncPromise<T> implements PromiseLike<T> {
private _state: States;
private _handlers: Array<[boolean, (value: T) => void, (reason: any) => any]>;
private _value: any;

public constructor(
executor: (resolve: (value?: T | PromiseLike<T> | null) => void, reject: (reason?: any) => void) => void,
) {
public constructor(executor: Executor<T>) {
this._state = States.PENDING;
this._handlers = [];

try {
executor(this._resolve, this._reject);
} catch (e) {
this._reject(e);
}
this._runExecutor(executor);
}

/** JSDoc */
/** @inheritdoc */
public then<TResult1 = T, TResult2 = never>(
onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | null,
onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | null,
Expand Down Expand Up @@ -99,14 +94,14 @@ class SyncPromise<T> implements PromiseLike<T> {
});
}

/** JSDoc */
/** @inheritdoc */
public catch<TResult = never>(
onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | null,
): PromiseLike<T | TResult> {
return this.then(val => val, onrejected);
}

/** JSDoc */
/** @inheritdoc */
public finally<TResult>(onfinally?: (() => void) | null): PromiseLike<TResult> {
return new SyncPromise<TResult>((resolve, reject) => {
let val: TResult | any;
Expand Down Expand Up @@ -138,35 +133,8 @@ class SyncPromise<T> implements PromiseLike<T> {
});
}

/** JSDoc */
private readonly _resolve = (value?: T | PromiseLike<T> | null) => {
this._setResult(States.RESOLVED, value);
Copy link
Member

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)

Copy link
Member Author

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!

Copy link
Member Author

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!

};

/** JSDoc */
private readonly _reject = (reason?: any) => {
this._setResult(States.REJECTED, reason);
};

/** JSDoc */
private readonly _setResult = (state: States, value?: T | PromiseLike<T> | any) => {
if (this._state !== States.PENDING) {
return;
}

if (isThenable(value)) {
void (value as PromiseLike<T>).then(this._resolve, this._reject);
return;
}

this._state = state;
this._value = value;

this._executeHandlers();
};

/** JSDoc */
private readonly _executeHandlers = () => {
/** Excute the resolve/reject handlers. */
private _executeHandlers(): void {
if (this._state === States.PENDING) {
return;
}
Expand All @@ -189,7 +157,38 @@ class SyncPromise<T> implements PromiseLike<T> {

handler[0] = true;
});
};
}
}

export { SyncPromise };
/** Run the executor for the SyncPromise. */
private _runExecutor(executor: Executor<T>): void {
const setResult = (state: States, value?: T | PromiseLike<T> | any): void => {
if (this._state !== States.PENDING) {
return;
}

if (isThenable(value)) {
void (value as PromiseLike<T>).then(resolve, reject);
return;
}

this._state = state;
this._value = value;

this._executeHandlers();
};

const resolve = (value: unknown): void => {
setResult(States.RESOLVED, value);
};

const reject = (reason: unknown): void => {
setResult(States.REJECTED, reason);
};

try {
executor(resolve, reject);
} catch (e) {
reject(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,7 @@ module.exports = {
create(context) {
return {
'ClassProperty, PropertyDefinition'(node) {
// We do allow arrow functions being initialized directly
if (
!node.static &&
node.value !== null &&
node.value.type !== 'ArrowFunctionExpression' &&
node.value.type !== 'FunctionExpression' &&
node.value.type !== 'CallExpression'
) {
if (node.value !== null) {
context.report({
node,
message: `Avoid class field initializers. Property "${node.key.name}" should be initialized in the constructor.`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member Author

Choose a reason for hiding this comment

The 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 🤔

Copy link
Member

Choose a reason for hiding this comment

The 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);
}
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { getMiddlewareSpanOptions, getNextProxy, instrumentObservable, isPatched
import type { CallHandler, CatchTarget, InjectableTarget, MinimalNestJsExecutionContext, Observable } from './types';

const supportedVersions = ['>=8.0.0 <11'];
const COMPONENT = '@nestjs/common';

/**
* Custom instrumentation for nestjs.
Expand All @@ -29,11 +30,6 @@ const supportedVersions = ['>=8.0.0 <11'];
* 2. @Catch decorator, which is applied on exception filters.
*/
export class SentryNestInstrumentation extends InstrumentationBase {
public static readonly COMPONENT = '@nestjs/common';
public static readonly COMMON_ATTRIBUTES = {
component: SentryNestInstrumentation.COMPONENT,
};

public constructor(config: InstrumentationConfig = {}) {
super('sentry-nestjs', SDK_VERSION, config);
}
Expand All @@ -42,7 +38,7 @@ export class SentryNestInstrumentation extends InstrumentationBase {
* Initializes the instrumentation by defining the modules to be patched.
*/
public init(): InstrumentationNodeModuleDefinition {
const moduleDef = new InstrumentationNodeModuleDefinition(SentryNestInstrumentation.COMPONENT, supportedVersions);
const moduleDef = new InstrumentationNodeModuleDefinition(COMPONENT, supportedVersions);

moduleDef.files.push(
this._getInjectableFileInstrumentation(supportedVersions),
Expand Down
6 changes: 3 additions & 3 deletions packages/react/src/errorboundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,14 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
}
}

public resetErrorBoundary: () => void = () => {
public resetErrorBoundary(): void {
const { onReset } = this.props;
const { error, componentStack, eventId } = this.state;
if (onReset) {
onReset(error, componentStack, eventId);
}
this.setState(INITIAL_STATE);
};
}

public render(): React.ReactNode {
const { fallback, children } = this.props;
Expand All @@ -164,7 +164,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
element = React.createElement(fallback, {
error: state.error,
componentStack: state.componentStack as string,
resetError: this.resetErrorBoundary,
resetError: this.resetErrorBoundary.bind(this),
eventId: state.eventId as string,
});
} else {
Expand Down
16 changes: 9 additions & 7 deletions packages/react/src/profiler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,6 @@ class Profiler extends React.Component<ProfilerProps> {
*/
protected _updateSpan: Span | undefined;

// eslint-disable-next-line @typescript-eslint/member-ordering
public static defaultProps: Partial<ProfilerProps> = {
disabled: false,
includeRender: true,
includeUpdates: true,
};

public constructor(props: ProfilerProps) {
super(props);
const { name, disabled = false } = this.props;
Expand Down Expand Up @@ -141,6 +134,15 @@ class Profiler extends React.Component<ProfilerProps> {
}
}

// React.Component default props are defined as static property on the class
Object.assign(Profiler, {
defaultProps: {
disabled: false,
includeRender: true,
includeUpdates: true,
},
});

/**
* withProfiler is a higher order component that wraps a
* component in a {@link Profiler} component. It is recommended that
Expand Down
10 changes: 1 addition & 9 deletions packages/replay-internal/src/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,8 @@ export const replayIntegration = ((options?: ReplayConfiguration) => {

/**
* Replay integration
*
* TODO: Rewrite this to be functional integration
* Exported for tests.
*/
export class Replay implements Integration {
/**
* @inheritDoc
*/
public static id: string = 'Replay';

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -114,7 +106,7 @@ export class Replay implements Integration {
beforeErrorSampling,
onError,
}: ReplayConfiguration = {}) {
this.name = Replay.id;
this.name = 'Replay';

const privacyOptions = getPrivacyOptions({
mask,
Expand Down
Loading
Loading