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

Missing @Injectable() or @Inject() decorators silently injects undefined #13899

Closed
4 of 15 tasks
glebbash opened this issue Aug 21, 2024 · 5 comments
Closed
4 of 15 tasks
Labels
needs triage This issue has not been looked into

Comments

@glebbash
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Missing @Injectable() or @Inject() decorators silently injects undefined.

This is a major issue as it does not prevent Nest.js application from starting and it's very easy to miss this in tests when mocking methods that use these broken injectables.

Notice the log output in the following repro:

Broken Working

Case 1

image

Case 2

image

Case 3

image

Case 1 is unexpected and should not happen / everyone needs to be aware of this problem.

Minimum reproduction code

https://github.com/glebbash/nest-injectable-repro

Steps to reproduce

  1. npm ci
  2. npm start
  3. Check logs

Expected behavior

There should be an error thrown, stopping the application from initializing or this issue must be documented.

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.1

Packages versions

[System Information]
OS Version     : Linux 6.1.91-060191-generic
NodeJS Version : v20.15.1
NPM Version    : 10.7.0 

[Nest CLI]
Nest CLI Version : 10.3.0 

[Nest Platform Information]
platform-express version : 10.4.1
schematics version       : 10.1.0
terminus version         : 10.2.2
swagger version          : 7.2.0
testing version          : 10.3.1
common version           : 10.3.1
axios version            : 3.0.1
core version             : 10.3.1
cli version              : 10.3.0

[Warnings]
The following packages are not in the same minor version
This could lead to runtime errors
* Under version 10.4
- @nestjs/platform-express 10.4.1
* Under version 10.3
- @nestjs/common 10.3.1
- @nestjs/core 10.3.1
* Under version 10.1
- @nestjs/schematics 10.1.0

Node.js version

20

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@glebbash glebbash added the needs triage This issue has not been looked into label Aug 21, 2024
@micalevisk
Copy link
Member

micalevisk commented Aug 21, 2024

this is expected due to TypeScript introspection limitations. I believe that you can use ESLint with plugins like https://github.com/darraghoriordan/eslint-plugin-nestjs-typed/ to detect such cases.

Neither @Inject() nor @Injectable() are mandatory if you won't inject anything on the provider, which is why nestjs itself couldn't prevent it.

@micalevisk micalevisk closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2024
@glebbash
Copy link
Author

glebbash commented Aug 21, 2024

But this lint plugin is not mentioned in the docs and does not come with default Nest.js installation.

@micalevisk I don't think it's very user friendly to not document known issues and just give link once to some third party solution in case someone stumbles upon this problem.

@micalevisk
Copy link
Member

micalevisk commented Aug 21, 2024

Got you. Few free to open a PR on the docs github repo to document that. There is a PR open already that covers such topic but maybe having a 'known limitations' page would be better

@jmcdo29
Copy link
Member

jmcdo29 commented Aug 21, 2024

For what it's worth, there is a PR that I want to have merged that talked about what the @Injectable() decorator does and why it's important

@glebbash
Copy link
Author

Linking this one because this issue happened before: #13059.

I might find some time to submit docs PR later.

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

3 participants