fix(core): Making Request scope non durable win over scope durable

[vizio360]

If an instance has at least one dependency that is Request scope non durable, then the instance should be Request scope non durable itself.

Closes #10594

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[micalevisk]

could you please open a PR at https://github.com/nestjs/docs.nestjs.com to clarify this new behavior?

[kamilmysliwiec]

@micalevisk PR to the docs doesn't seem to be needed (it should work as intended upon merging this PR)

[vizio360]

@kamilmysliwiec @micalevisk I do wonder if I also need to check the Dynamic Modules that use a factory. With my tests it seems that if at least one injected dependency to the factory method is durable then that factory method becomes durable even though it has some other dependencies that are Request non durable.

[vizio360]

@kamilmysliwiec @micalevisk I do wonder if I also need to check the Dynamic Modules that use a factory. With my tests it seems that if at least one injected dependency to the factory method is durable then that factory method becomes durable even though it has some other dependencies that are Request non durable.

Actually I need to dig a bit further because I see the problem while using @golevelup/nestjs-graphql-request

[vizio360]

@kamilmysliwiec @micalevisk I do wonder if I also need to check the Dynamic Modules that use a factory. With my tests it seems that if at least one injected dependency to the factory method is durable then that factory method becomes durable even though it has some other dependencies that are Request non durable.

Actually I need to dig a bit further because I see the problem while using @golevelup/nestjs-graphql-request

Ok I think I spotted the issue in my tests. The .overrideProvider().useValue() sequence seems to set the Scope for the provider you pass as DEFAULT but keeps the durable flag as set in the original provider class.

My class definition

@Injectable({ scope: Scope.REQUEST, durable: false })
export class M2MTenantAuthService {

in my test

.overrideProvider(M2MTenantAuthService).useValue(m2mTokenMock)

and this is the debug data I got for that class

Notice the durable: false (as specified in the class definition) but the scope: 0

Wondering if because I use useValue it automatically assumes it is scope DEFAULT.

Anyway nothing to add to this PR.

[micalevisk]

yeah, looks like there's no way to define the scope of overwrited providers when using .useValue. I can't tell if this is a limitation or not

nest/packages/testing/testing-module.builder.ts

Lines 91 to 95 in eebb976

	private createOverrideByBuilder(
	add: (provider: any) => TestingModuleBuilder,
	): OverrideBy {
	return {
	useValue: value => add({ useValue: value }),

[vizio360]

yeah, looks like there's no way to define the scope of overwrited providers when using .useValue. I can't tell if this is a limitation or not

nest/packages/testing/testing-module.builder.ts

Lines 91 to 95 in eebb976

	private createOverrideByBuilder(
	add: (provider: any) => TestingModuleBuilder,
	): OverrideBy {
	return {
	useValue: value => add({ useValue: value }),

In theory it should maintain the scope defined in the original provider.

[vizio360]

@kamilmysliwiec any idea when this can be merged and released?

[vizio360]

@micalevisk @kamilmysliwiec @Papooch just a little nudge to know when we can expect this to be merged.

[micalevisk]

@vizio360 can you patch this to see if it will fix the issue described at #10809 as well? If yes, I'll close that PR

[vizio360]

@vizio360 can you patch this to see if it will fix the issue described at #10809 as well? If yes, I'll close that PR

I can have a look, but is that PR not ready to be merged? Is it just a matter of including those changes in this one or to actually fix the problem that PR was created for?

[micalevisk]

it is but I didn't notice before that maybe yours would fix that issue as well but in a more cleaner way.

To clarify: I'm not asking you to bring those changes to here, but to check out if the bug described in there will be fixed by this PR.

[vizio360]

it is but I didn't notice before that maybe yours would fix that issue as well but in a more cleaner way.

To clarify: I'm not asking you to bring those changes to here, but to check out if the bug described in there will be fixed by this PR.

sure, I'll have a look.

[vizio360]

Just found that the enhancers tests are failing. Investigating...

[vizio360]

@vizio360 can you patch this to see if it will fix the issue described at #10809 as well? If yes, I'll close that PR

Forgot to update on this, I checked and this PR does not solve the issue that #10809 is addressing.
#10809 fix seems ok to me.

[kamilmysliwiec]

lgtm

[micalevisk]

seems like this has introduced a regression: #11002