docs.nestjs.com

docs.nestjs.com copied to clipboard

Is there an existing issue that is already proposing this?

• [X] 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.

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.

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

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.

what do you guys think: https://github.com/nestjs/docs.nestjs.com/pull/2944

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

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.