getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.76k stars 1.52k forks source link

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

Open DanielMenke opened 1 week ago

DanielMenke commented 1 week ago

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

```typescript
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 commented 1 week ago

Hi, thanks for writing in. I am a bit confused tbh. We are only referencing the __wrapped field in https://github.com/getsentry/sentry-javascript/blob/c7b8503018f9384bb4f68a2c3f86c43956fa8380/packages/aws-serverless/src/sdk.ts#L374-L377

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 commented 1 week ago

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.

lforst commented 1 week ago

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

DanielMenke commented 1 week ago

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

DanielMenke commented 1 week ago

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 commented 4 days ago

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.