nest icon indicating copy to clipboard operation
nest copied to clipboard

draft: fix(common): change configurable module class type to support promise

Open jmcdo29 opened this issue 3 years ago • 6 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [X] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [X] Tests for the changes have been added (for bug fixes / features)
  • [X] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [X] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

Configurable modules built with the ConfigurableModuleBuilder can't have async register or registerAsync methods

Issue Number: #10226

What is the new behavior?

Configurable modules built with the ConfigurableModuleBuilder can have async register or registerAsync methods

Does this PR introduce a breaking change?

  • [ ] Yes
  • [X] No

Other information

jmcdo29 avatar Sep 03 '22 21:09 jmcdo29

Ah, shoot, this gets more tricky when it comes to spreading the result. I'll need to play around with this more to see if we can get this extension to really work. I think there should be a way to do this, make the function optionally async, but it's gonna be weird on the Typescript side of things

jmcdo29 avatar Sep 03 '22 21:09 jmcdo29

Strange that it fails the build was not modified this file:

Schermata 2022-09-04 alle 16 10 10

🤔

Tony133 avatar Sep 04 '22 14:09 Tony133

@Tony133 is makes sense because we use a ...super.forRoot() and ...super.forRootAsync() and Typescript isn't able to immediately guarantee that the return of T | Promise<T> is not going to be that Promise<T>. We know better, because we aren't doing anything async in the function, but if we want to support the possibility of the methods returning a Promise<T> then we need to figure out some way to appease Typescript here. I'd rather not resort to an as T assertion, but that may be the solution.

If that is the solution though, this could be seen as a breaking change, because if anyone else is already doing that same kind of spread then their build will fail when we publish this change.

jmcdo29 avatar Sep 04 '22 17:09 jmcdo29

Very interesting, thank you very much for the explanation 👍

Tony133 avatar Sep 04 '22 19:09 Tony133

Is this PR ready for review 🤔 ? @jmcdo29

kamilmysliwiec avatar Feb 01 '23 12:02 kamilmysliwiec

No. There's still a type issue that I'm trying to work back out

jmcdo29 avatar Feb 01 '23 14:02 jmcdo29