TriPSs / nestjs-query

Easy CRUD for GraphQL.
https://tripss.github.io/nestjs-query/
MIT License
164 stars 46 forks source link

fix: Corrected missing promises in functions that should have been present #242

Closed Copystrike closed 8 months ago

Copystrike commented 8 months ago

In the AbstractAssembler, there are several methods that provide the option to be async.

https://github.com/TriPSs/nestjs-query/blob/b5c761bd42ceccfd8d68e34722706b463b31d73c/packages/core/src/assemblers/abstract.assembler.ts#L37-L39

https://github.com/TriPSs/nestjs-query/blob/b5c761bd42ceccfd8d68e34722706b463b31d73c/packages/core/src/assemblers/abstract.assembler.ts#L47-L49

However, in the ClassTransformerAssembler class, when extending these abstract methods, we explicitly set return types to one of the two return type options:

https://github.com/TriPSs/nestjs-query/blob/b5c761bd42ceccfd8d68e34722706b463b31d73c/packages/core/src/assemblers/class-transformer.assembler.ts#L20-L26

https://github.com/TriPSs/nestjs-query/blob/b5c761bd42ceccfd8d68e34722706b463b31d73c/packages/core/src/assemblers/class-transformer.assembler.ts#L40-L46

Which cause this in my project:

image

Which prohibited the use of the async option.

When I change the types:

image

Everything works as expected:

image

Which is what I hope this PR achieved!

TriPSs commented 8 months ago

Cool, thanks!

nx-cloud[bot] commented 8 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c4d712d634a4ba5d3ea0760ba162d40e33fbec78. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 5 targets - [`nx run-many --target=e2e --all`](https://cloud.nx.app/runs/iO06iNaKb3?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=lint --all`](https://cloud.nx.app/runs/3LXLsDaT5K?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=build --all`](https://cloud.nx.app/runs/kg9oBMQUQ0?utm_source=pull-request&utm_medium=comment) - [`nx run workspace:version`](https://cloud.nx.app/runs/721fYBNcPP?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=test --all`](https://cloud.nx.app/runs/fQurgmYn5m?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.27%. Comparing base (6bb3bdd) to head (c4d712d). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #242 +/- ## ========================================== + Coverage 86.19% 86.27% +0.08% ========================================== Files 688 688 Lines 9765 9765 Branches 864 864 ========================================== + Hits 8417 8425 +8 + Misses 626 619 -7 + Partials 722 721 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Copystrike commented 8 months ago

Ahh, some of the test failed, my bad. Thought this was going to be an easy fix, will see if I can get this fixed asap.

Copystrike commented 8 months ago

I might need some assistance with this problem. Basically, the issue is that in ClassTransformerAssembler, the types are set explicitly. This causes trouble when trying to use async functions because TypeScript can't figure out if users want a promise or the actual type.

So, if people don't use async, they have to specify the type they want like this:

  convertToDTO(entity: TodoItemEntity): TodoItemDTO {
    const dto = super.convertToDTO(entity) as TodoItemDTO
    dto.age = Date.now() - entity.createdAt.getMilliseconds()
    return dto
  }

This could lead to breaking changes in the type checking process, because it forces changes in how things are done.

Another idea is to create a new class called AsyncClassTransformerAssembler. But I'm not a fan of this because it means more work to maintain and limits using both async and non-async functions in a single class.

TriPSs commented 8 months ago

Let's just update it and mark it a breaking change. Don't forget to add BREAKING CHANGE: into the commit body like here

Copystrike commented 8 months ago

Let's just update it and mark it a breaking change. Don't forget to add BREAKING CHANGE: into the commit body like here

Done, breaking change notice is included.

Normally this time it should be able to pass the tests!