Skip to content
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

Sentry sdk adds "__wrapped"-properties to types, which prevents nestjs project from building #12682

Open
3 tasks done
DanielMenke opened this issue Jun 27, 2024 · 6 comments · Fixed by open-telemetry/opentelemetry-js#4865
Assignees

Comments

@DanielMenke
Copy link

DanielMenke commented Jun 27, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/nestjs

SDK Version

8.1.3.0

Framework Version

10.0.3

Link to Sentry event

No response

SDK Setup/Reproduction Example

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

Sentry.init({
  dsn: 'X',
});
async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  const { httpAdapter } = app.get(HttpAdapterHost);
  Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));

 ...

  await app.listen(4000);
}

bootstrap();

Steps to Reproduce

  1. Use a custom Type which makes all properties of a type required:
export type Complete<T> = {
  [P in keyof Required<T>]: Pick<T, P> extends Required<Pick<T, P>>
    ? T[P]
    : T[P] | undefined;
};

in

export function UpdateContext<
  Model extends BaseModel,
  Dto extends Complete<Type<ContextAwareInput<GqlQueryContext>>>,
>(dto: Dto) {
  @ArgsType()
  class Context extends ContextAwareInput<GqlQueryContext> {
    @Field(() => ID)
    id: Model['id'];

    @Field(() => dto)
    dto: Dto;
  }

  return Context as Type<UpdateContext<Model, Dto>>;
}

and

async updateTreatment(
    @Update(UpdateInput) context: UpdateContext<Model, UpdateInput>
  ) {
    const entity = await this.service.findOneByContext(context);
    if (!entity) throw new NotFoundGqlError('Treatment not found');

    return this.service.updateByContext(entity, context);
  }
  1. Install @sentry/node

  2. run nest build

Expected Result

NestJs builds without Problems

Actual Result

The SDK expects the silently added __wrapped-property to be defined and Typescript throws the following Error:

TS2345: Argument of type 'typeof UpdateTreatmentInput' is not assignable to parameter of type 'Complete<Type<ContextAwareInput<GqlQueryContext>>>'.
  Property '__wrapped' is optional in type 'typeof UpdateTreatmentInput' but required in type 'Complete<Type<ContextAwareInput<GqlQueryContext>>>'.
@lforst
Copy link
Member

lforst commented Jun 28, 2024

Hi, thanks for writing in. I am a bit confused tbh. We are only referencing the __wrapped field in

function isWrappedByOtel(
// eslint-disable-next-line @typescript-eslint/ban-types
handler: Function & { __original?: unknown; __unwrap?: unknown; __wrapped?: boolean },
): boolean {

However, that type is not exposed in any way. Are you 100% sure Sentry is causing this? Are you referencing our internals in any weird way?

@DanielMenke
Copy link
Author

Hi, thanks for the quick response. Unfortunately it only occurs when adding Sentry to the project, without any "special" configuration. After further investigation, I could pin it down to this peerDependency: shimmer 1.05, which is used by openTelegraphy 1.16.0. So it isn't directly caused by Sentry, but rather by the used dependencies.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 28, 2024
@lforst
Copy link
Member

lforst commented Jun 28, 2024

@DanielMenke interesting. Thanks for investigating. Could you elaborate where exactly the error points to? We'll try to report and fix this upstream.

@DanielMenke
Copy link
Author

DanielMenke commented Jun 28, 2024

The error is complaining that the input UpdateInput given to the function-parameter decorator @Update() has an optional __wrapped-property. This does not satisfy the type Complete<T> as this type requires every property of a given type to be defined. As the __wrapped-property is magically injected into every type in the project(correct me if thats a wrong assumption), the functionality of the Complete<T>-type breaks, or gets as least really cumbersome, as it requires the __wrapped-property to be defined everywhere.

I created a minimal reproduction repo here(I removed the graphql focus to keep it minimal):
https://github.com/DanielMenke/sentry_min_reproduction

I also added a separate branch without any sentry-relations, which starts without any problems (you might need to delete the node_modules directory upfront)
https://github.com/DanielMenke/sentry_min_reproduction/tree/no-sentry

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jun 28, 2024
@DanielMenke DanielMenke changed the title Sentry sdk adds "__wrapper"-properties to types, which prevents nestjs project from building Sentry sdk adds "__wrapped"-properties to types, which prevents nestjs project from building Jun 28, 2024
@DanielMenke
Copy link
Author

DanielMenke commented Jun 28, 2024

Update: The Problem also occurs with the typescript-native Utility-Type Required<T> when using it like this: Required<Type<T>>. The Type<T> is an interface originating from @nestjs/common and enables class-types to be passed as function-parameters instead of an actual instance of given class.

Current workaround would be to use Omit<Required<Type<T>>, '__wrapped'> instead.

@lforst
Copy link
Member

lforst commented Jul 1, 2024

Thanks a bunch for the investigation and reproduction! Grade A issue 👌

I opened a bunch of upstream contributions so we can fix this. For now, I recommend a good ol' @ts-ignore as a workaround until all of the changes are released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment