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

Better docs for implicit request scoped providers #2945

Open
1 task done
StiliyanKushev opened this issue Jan 22, 2024 · 7 comments
Open
1 task done

Better docs for implicit request scoped providers #2945

StiliyanKushev opened this issue Jan 22, 2024 · 7 comments

Comments

@StiliyanKushev
Copy link

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

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.

import { Inject, Injectable, Scope } from '@nestjs/common';
import { REQUEST } from '@nestjs/core';

// @Injectable({ scope: Scope.REQUEST })
@Injectable() // the provider still becomes request scoped
export class MyProviderClass {
  constructor(
    @Inject(REQUEST) // 👈
    private readonly requestContext: unknown,
  ) {}
}

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.

@micalevisk
Copy link
Member

micalevisk commented Jan 22, 2024

I guess we should mention that here: https://docs.nestjs.com/fundamentals/injection-scopes#request-provider

@StiliyanKushev
Copy link
Author

StiliyanKushev commented Jan 22, 2024

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.

@jmcdo29
Copy link
Member

jmcdo29 commented Jan 22, 2024

We do have this mentioned

The REQUEST scope bubbles up the injection chain. A controller that depends on a request-scoped provider will, itself, be request-scoped.

If you think explicitly mentioning that this overrides the @Injectable({scope}), then we can add it

@StiliyanKushev
Copy link
Author

I guess you're right about the docs mentioning the bubbling-up effect, but one has to realize that the REQUEST token points to a custom internal provider that itself is request scoped by definition. (which now that you mention it, duh..)

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.

@micalevisk
Copy link
Member

what do you guys think: #2944

@jmcdo29
Copy link
Member

jmcdo29 commented Jan 22, 2024

If you think it would help to mention that the REQUEST token is REQUEST scoped, then I think it would be good to add it. I'm well aware of this stuff, so it doesn't necessarily hit my radar of things that should be mentioned, but we're always looking to make the docs as user friendly as we can

@StiliyanKushev
Copy link
Author

StiliyanKushev commented Jan 22, 2024

If you think explicitly mentioning that this overrides the @Injectable({scope}), then we can add it

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 scope option (which gets overridden when you inject a request-scoped provider), the durable option, which also bubbles-up similarly, can actually be enforced, meaning the parent provider can set it to false, canceling out the bubbling-up effect.

If that's true, then this is a discrepancy in the behavior of those two options and might benefit from slight documentation improvements.

@kamilmysliwiec kamilmysliwiec transferred this issue from nestjs/nest Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants