-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Better docs for implicit request scoped providers #2945
Comments
I guess we should mention that here: https://docs.nestjs.com/fundamentals/injection-scopes#request-provider |
Don't really wish to be nitpicking, but unlike the above example, shouldn't this throw an error? import { Inject, Injectable, Scope } from '@nestjs/common';
import { REQUEST } from '@nestjs/core';
@Injectable({ scope: Scope.DEFAULT })
export class MyProviderClass {
constructor(
@Inject(REQUEST) // 👈
private readonly requestContext: unknown,
) {}
} Because as of right now the provider still becomes request-scoped, rendering the additional decorator's scope parameter useless. |
If you think explicitly mentioning that this overrides the |
I guess you're right about the docs mentioning the bubbling-up effect, but one has to realize that the I guess that's what people like me might find a little bit confusing, but could very well just be my unfamiliarity with the framework talking. |
what do you guys think: #2944 |
If you think it would help to mention that the |
I think this could also be a good addition to the docs, because as far as I understand it (correct me if I'm wrong), unlike the If that's true, then this is a discrepancy in the behavior of those two options and might benefit from slight documentation improvements. |
Is there an existing issue that is already proposing this?
Is your feature request related to a problem? Please describe it
I believe the docs never mention the implicit behavior of a provider automatically becoming request scoped if it injects the
REQUEST
token in its constructor.Describe the solution you'd like
I'd like to see this mentioned in the official docs if it's expected behavior.
Teachability, documentation, adoption, migration strategy
Alternatively, have nest throw an error (or at least a warning) in such cases if it's not expected behavior.
What is the motivation / use case for changing the behavior?
IMO, this is rather unintuitive at a first glance, and may potentially cause confusion.
The text was updated successfully, but these errors were encountered: