tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

mixin-based multiple inheritance not working with nestjs/swagger decorators #34

Closed ishandrov closed 2 years ago

ishandrov commented 3 years ago

Hi there,

Let me try to describe the issue with ts-mixer I'm facing right now. So I'm using nestjs as the main dev framework for the backend and currently implementing the register method that would allow the application to register all types of users. There're a few possible types of users here. Some of them are FAN, FIGHTER, JUDGE etc. Each user has its own set of specific parameters. For example, FIGHTER has such params as height, weight, wins etc that are specific to fighters only.

So far the register method looks like this:

    @Post('register')
    public async registerUser(@Body() registerUserDto: RegisterUserDto): Promise<void> {
        // code
    }

That RegisterUserDto is supposed to contain all the fields coming from all types of users, but frontend-wise, when I register a specific user I should be allowed to specify only those json fields relevant to the type of user I'm registering. It means that the majority of fields defined on RegisterUserDto will be optional. The thing is that, in order to improve code readability and code maintenance, I would like to have separate dtos for each possible type of user and then pile them all together into a single object (dto) using multiple inheritance or the intersection type. The second option does not work cos in that case the class-validator and nestjs/swagger libs stop seeing their decorators. The first option seems to be sort of doable thanks to the ts-mixer library, cos as it's stated in its documentation we should wrap validation decorators in the @decortor decorator. It works like a charm, but...it's not the case with the nestjs/swagger decorators and that's the problem I'm trying to solve. For instance, when I use @ApiProperty decorator on any of the defined props I get this error:

(node:22654) UnhandledPromiseRejectionWarning: Error: A circular dependency has been detected (property key: "firstName"). Please, make sure that each side of a bidirectional relationships are using lazy resolvers ("type: () => ClassType").
    at SchemaObjectFactory.createNotBuiltInTypeReference (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:212:19)
    at SchemaObjectFactory.mergePropertyWithMetadata (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:143:25)
    at /Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:79:35
    at Array.map (<anonymous>)
    at SchemaObjectFactory.extractPropertiesFromType (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:78:52)
    at SchemaObjectFactory.exploreModelSchema (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:92:41)
    at /Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:33:36
    at Array.map (<anonymous>)
    at SchemaObjectFactory.createFromModel (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/services/schema-object-factory.js:20:45)
    at exploreApiParametersMetadata (/Users/albert/Documents/projects/albert/rlx/node_modules/@nestjs/swagger/dist/explorers/api-parameters.explorer.js:33:55)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:22654) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:22654) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Here is more code:

export class RegisterCommonUserDto {
    @decorate(ApiProperty())
    @decorate(IsNotEmpty())
    @decorate(IsString())
    firstName: string;

    // @decorate(ApiProperty())
    @decorate(IsNotEmpty())
    @decorate(IsString())
    lastName: string;

    // @decorate(ApiProperty({enum: EUserRoleName}))
    @decorate(IsNotEmpty())
    @decorate(IsEnum(EUserRoleName))
    roleName: EUserRoleName;

    // @decorate(ApiProperty())
    @decorate(IsNotEmpty())
    @decorate(IsPhoneNumber())
    phoneNumber: string;

    // @decorate(ApiProperty())
    @decorate(IsNotEmpty())
    @decorate(IsEmail())
    email: string;
}

and the RegisterUserDto looks as follows:

export class RegisterUserDto extends Mixin(
    RegisterFighterDto,
    RegisterCommonUserDto,
    RegisterLocationProviderDto,
) {}

I would appreciate your help!

tannerntannern commented 3 years ago

Hi @ishandrov, thanks for your interest in ts-mixer and apologies for the puzzling issue. I'm generally a little hesitant to dive into issues related to 3rd party libraries because I couldn't possibly support every one. I'm especially hesitant in this case since it's not clear to me that ts-mixer is the root of the issue here. I am familiar with Swagger in general, but not the integration provided by NestJS. If I were to troubleshoot this issue, I would need a minimal reproducible example repo at a minimum. If the issue could be narrowed down to a specific decorator/scenario, that would be even better. My time for supporting this package is very limited these days...

Might be easiest to fork this repo and create a test file under test/gh-issues. Then you can verify that your minimal reproducible example works (or is broken rather, haha) with yarn test.

The test file for issue #15 might be similar to what you need to do.

ishandrov commented 3 years ago

Thank you for the quick reply.

Ah, I totally understand you. I agree it's not really clear where this issue stems from, so yeah...I got sort of desperate when I stumbled across this problem, I thought I had found a great solution but then the swagger fired off this error and the whole world came crushing down...Anyway, I gave up on that inheritance thing, at least for now and went for a single dto for all types of users.

Yeah when I have time I will create a MRER so that you could reproduce it. When I do this I will let you know.

P.S.

To be honest, I'm more inclined to believe that it's the swagger issue, not entirely sure though, but I've never liked that wrapper and its lack of documentation, seems like it was cobbled together in a very hasty manner...But I would very much appreciate it if you would help me shed some light on this mystery.

TrejGun commented 2 years ago

this was tested in https://github.com/tannerntannern/ts-mixer/issues/35

tannerntannern commented 2 years ago

@TrejGun Can you clarify what you mean here? Is this issue good to close from your perspective? or do I need to keep it open?

TrejGun commented 2 years ago

good to be closed. thanks.