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

Add mixins (extends) feature to work with @nestjs/mapped-types and @nestjs/swagger #271

Closed
micalevisk opened this issue Mar 6, 2021 · 15 comments
Labels
enhancement New feature or request

Comments

@micalevisk
Copy link
Contributor

Hey!

I want to use @automapper/nestjs along with @nestjs/swagger to be able to generate an OpenAPI spec from my DTOs (or view models).

However the mapped types mixins from @nestjs/swagger (the same of @nestjs/mapped-types) do not inherit automapper's metadata, as you already notice here: #173 (comment) thus this following code will not work as expected:

import { ApiPropertyOptional, OmitType } from '@nestjs/swagger';

class UserVm { // assuming that each prop can have some kind of ApiProperty decorator
  // ...
  @AutoMap()
  @ApiProperty() // this metadata (and any other that ) should be inherited
  name string

  @AutoMap()
  @ApiPropertyOptional({ type: String, format: 'date-time' })
  deletedAt?: Date
  // ...
}

class NonDeletableUserVm extends OmitType(UserVm, ['deletedAt']) {}
// ^ this class will be a swagger schema/model as well

// ...
mapper.createMap(User, UserVm)
mapper.createMap(User, NonDeletableUserVm)

ie. when mapping User to NonDeletableUserVm we'll get NonDeletableUserVm {}

Therefore, automapper should have mixins like PickType and OmitType to make mapper.map(user, NonDeletableUserVm, User) return NonDeletableUserVm { name: "foo" } instead of NonDeletableUserVm {} without removing metadata that are copied in @nestjs/swagger


Related issue: nestjsx/automapper#254

@nartc
Copy link
Owner

nartc commented Mar 7, 2021

I have some stashed code for this which could give me a headstart. However, I'm thinking of which package we'd want to publish this under:

  • @automapper/classes would make sense because this is the package that provides AutoMap and you only really use mapped-types with AutoMap decorator.
  • @automapper/nestjs would also make sense
  • @automapper/mapped-types would also make sense. But this would now require the consumers to install another package. I don't really want to turn into another @angular/* lol

What do you think? I'm leaning towards @automapper/classes

@micalevisk
Copy link
Contributor Author

micalevisk commented Mar 7, 2021

I've said @automapper/nestjs before but now I notice that one might use these mixins in any other project that uses class-validator or class-transformer. Thus @automapper/classes sounds better.

To me @automapper/mapped-types make more sense if this feature increase significantly the bundle size of @automapper/classes (which is 7.7 kB now)

@micalevisk micalevisk changed the title [nestjs] Add mixins (extends) feature to work with @nestjs/mapped-types and @nestjs/swagger Add mixins (extends) feature to work with @nestjs/mapped-types and @nestjs/swagger Mar 7, 2021
@nartc
Copy link
Owner

nartc commented Mar 8, 2021

@micalevisk 7.7kb is nothing if used on the server-side (client-side can benefit better with gzipped + minified). And as soon as Nx supports differential build for Node Package then tree-shaking is also available.

@nartc
Copy link
Owner

nartc commented Mar 12, 2021

https://automapperts.netlify.app/docs/plugins-system/classes-mapped-types give this a try in v3.3.0. I don't close this issue yet since not sure if it works for you.

@nartc nartc added the enhancement New feature or request label Mar 13, 2021
@micalevisk
Copy link
Contributor Author

Looks like the mappings are working but it still not inherits the metadata from @nestjs/swagger, even when I'm explicity use the @ApiProperty decorator (and not the CLI plugin) 😕

class Foo {
  @AutoMap()
  foo: string;

  @AutoMap()
  @ApiProperty({ description: 'do something' }) // or any other `Api`-like decorators
  bar: number;
}

class SubFoo extends MapperOmitType(Foo, ['foo']) {}

If you can somehow make this work with the OpenAPI CLI plugin (from Nestjs) as well it would be awesome!

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

Since there’re mixins, can you just compose them?

Like OmitType(MapperOmitType()) kinda?

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

Well, it's probably not possible. I'll give it more thoughts tomorrow.

@micalevisk okay, I'm at a roadblock. I'm not sure how to combine these mixins between packages since the typings and the parameters types are all different, and there is also an unknown number of mixins that you'd combine. You have any idea?

@micalevisk
Copy link
Contributor Author

Looking at @nestjs/swagger's source I realised that to make your mixins work with @nestjs/swagger @ApiProperty() decorator, you must do something like this before returning the class. Thus @automapper/classes should know, in some sort, about @nestjs/swagger

I've tried the following version of MapperPickType and it works:

export function MyMapperPickType<T, K extends keyof T>(
  classRef: Constructible<T>,
  keys: readonly K[]
): MappedType<Pick<T, typeof keys[number]>> {
  const isInheritedPredicate = (propertyKey: string) =>
    keys.includes(propertyKey as K);

  class PickClassType {
    constructor() {
      inheritPropertyInitializers(
        this as Record<string, unknown>,
        classRef,
        isInheritedPredicate
      );
    }
  }

  inheritAutoMapMetadata(classRef, PickClassType, isInheritedPredicate);

  // ---------------------------------------------------------------------- //
  clonePluginMetadataFactory(
    PickClassType,
    classRef.prototype,
    (metadata: Record<string, any>) => pick(metadata, keys)
  );
  // ---------------------------------------------------------------------- //

  return PickClassType as MappedType<Pick<T, typeof keys[number]>>;
}
my set up

UserRoDto

UserStrictRoDto

Using MapperPickType

using automapper mixin

Using MyMapperPickType

using my mixin

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

Yeah, that's what I want to avoid, I don't want automapper/classes to depend on nestjs/swagger :(

@micalevisk
Copy link
Contributor Author

I'm with you.

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

So can you confirm if something like this would work?

export function MyPickType<T, K extends keyof T>(
  classRef: Constructible<T>,
  keys: readonly K[]
): MappedType<Pick<T, typeof keys[number]>> {

   class MappedClass extends MapperPickType(classRef, keys) {}
  // ---------------------------------------------------------------------- //
  clonePluginMetadataFactory(
    MappedClass,
    classRef.prototype,
    (metadata: Record<string, any>) => pick(metadata, keys)
  );
  // ---------------------------------------------------------------------- //

  return MappedClass as MappedType<Pick<T, typeof keys[number]>>;
}

This way, we can reuse MapperPickType instead of having to rewrite the whole thing?

@micalevisk
Copy link
Contributor Author

micalevisk commented Mar 14, 2021

I'm getting TS error in MapperPickType(classRef, keys): Base constructor return type 'Pick<T, K>' is not an object type or intersection of object types with statically known members.ts(2509) due to

(alias) MapperPickType<T, K>(classRef: Constructible<T>, keys: readonly K[]): MappedType<Pick<T, K>>
import MapperPickType

but it works if I ignore this error.

The problem with that is using clonePluginMetadataFactory which is not exposed by @nestjs/swagger -- I'm importing this with @nestjs/swagger/dist/type-helpers/mapped-types.utils tho

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

does adding as unknown remove the type error?

@nartc
Copy link
Owner

nartc commented Mar 14, 2021

hm...so we're using a private API then. I guess I should halt on this since I'm not sure how to move forward. If you think of a stable workaround, please let me know so I can add to the docs.

@MinaFayez9
Copy link

MinaFayez9 commented Feb 8, 2023

@nartc
I faced the same issue and tried the following as a workaround.
So, does it look like a valid and clean workaround for you or a rather hacky one?

get-organization.dto.ts

export class GetOrganizationDto extends OmitType<
  Organization,
  (typeof GET_ORGANIZATION_OMIT_PROPERTIES)[number]
>(Organization, GET_ORGANIZATION_OMIT_PROPERTIES) {}

organization.mapper.ts

class GetOrganizationAutomapperDto extends MapperOmitType<
  Organization,
  (typeof GET_ORGANIZATION_OMIT_PROPERTIES)[number]
>(Organization, GET_ORGANIZATION_OMIT_PROPERTIES) {}

@Injectable()
export class OrganizationMapper extends AutomapperProfile {
  constructor(@InjectMapper() mapper: Mapper) {
    super(mapper);
  }

  override get profile(): MappingProfile {
    return (mapper: Mapper) => {
      createMap(mapper, Organization, GetOrganizationAutomapperDto);
      createMap(
        mapper,
        Organization,
        GetOrganizationDto,
        extend<
          Organization,
          GetOrganizationDto,
          Organization,
          GetOrganizationDto
        >(Organization, GetOrganizationAutomapperDto),
      );
    };
  }
}

organizations.controller.ts

@Controller('organizations')
export class OrganizationsController {
  constructor(
    private readonly organizationsService: OrganizationsService,
    @InjectMapper() private readonly mapper: Mapper,
  ) {}

  @Get(':id')
  async findById(@Param('id') id: string): Promise<GetOrganizationDto> {
    const organization = await this.organizationsService.findById(id);
    return this.mapper.map(
      organization,
      Organization,
      GetOrganizationDto,
    );
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants