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

NestJS doesn't error when provider's dependencies haven't been resolved #13059

Closed
6 of 15 tasks
snigdha920 opened this issue Jan 16, 2024 · 5 comments
Closed
6 of 15 tasks
Labels
needs triage This issue has not been looked into

Comments

@snigdha920
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

We have the case where:

  1. We create a service without an @Injectable decorator on the service class, let's call this BookService
  2. We include the service in the providers array of a module BookModule
  3. We use the service in a controller BookController

The BookService injects another service AuthorService, but bookService.authorService is undefined and nest does not error out - no errors on the BookService or BookController (which is using the service)

In a unit test, importing the BookModule and then doing

service = module.get<BookService>(BookService)

service is defined. but bookService.authorService is undefined, because it was not injected correctly.

Minimum reproduction code

https://github.com/snigdha920/nestjs-di

Steps to reproduce

Steps to reproduce

  1. Install dependencies with pnpm i
  2. Run pnpm start:dev in the root
  3. No errors in the console, but we see the message from the BookService constructor: BookService.authorService is undefined
  4. In the test book.service.spec.ts, we successfully get the BookService from the module context, but bookService.authorService is undefined. Run the test by pnpm test

Expected behavior

This behaviour is weird, what I expect is:

Since BookService is included in the providers array, nest should throw an error can't resolve dependancies of BookService

OR

Since BookService is being used by the BookController, the BookController should throw an error that it couldn't resolve the BookService dependency since the dependencies of BookService itself hasn't been resolved

OR

The unit test to should error saying it could not find the provider BookService in the module context since BookService hasn't been initialised properly because it's dependencies weren't resolved

I basically want some error at compile time that we're using a service in a controller whose dependencies have not been resolved. This service is also included in the providers array, so there should be some error at compile time.

Is there a unit test I can write that catches this? Instead of having to manually check for every dependency of a service in a unit test (which I cannot do if BookService.authorService was a private field)

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

10.3.0

Packages versions

"dependencies": {
    "@nestjs/common": "10.3.0",
    "@nestjs/core": "10.3.0",
    "@nestjs/platform-express": "10.3.0"
  },

Node.js version

20.10.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@snigdha920 snigdha920 added the needs triage This issue has not been looked into label Jan 16, 2024
@micalevisk
Copy link
Member

micalevisk commented Jan 16, 2024

is undefined and nest does not error out

I believe that this is a limitation of typescript, and could be handled by a linter rule

We can add classes without the @Injectable() to the providers array as long as they won't inject anything or if they're using the @Inject() instead

There is no way to nestjs to know if you missed the @Injectable() or not, due to how typescript introspection works (more on this here: nestjs/docs.nestjs.com#2481)

@snigdha920
Copy link
Author

We can add classes without the @Injectable() to the providers array as long as they won't inject anything or if they're using the @Inject() instead

Why is this allowed? Allowing classes to be in the providers without @Injectable can allow them to have unresolved dependencies, which are only caught at runtime. Is there a case where want to put a class in the providers array but don't want it to be injectable?

@jmcdo29
Copy link
Member

jmcdo29 commented Jan 16, 2024

The @Injectable() decorator is a way to force Typescript to emit the constructor metadata for what parameters are there. If the class doesn't have parameters, you technically don't need the decorator, so adding the class without the decorator to the providers is perfectly valid. Regardless if the class is decorated, it does still need to be added to the providers array so nest can be aware of the existence of the class and add it to the provider tokens map

@snigdha920
Copy link
Author

Okay, also read https://github.com/nestjs/docs.nestjs.com/pull/2481/files. So nest can't possibly resolve dependencies of a class (with a non-empty constructor) in the providers array without at least a @Inject / @Injectable decorator because there won't be enough metadata to do so. Is that correct?

@micalevisk
Copy link
Member

@snigdha920 exactly

and nestjs can't tell if a class has a non-empty constructor if the class has no decorators. That's why I believe this is a limitation we can't fix on nestjs side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

4 participants