request property in a service class is undefined after v9.3.1 #11002
Comments
karam-mustafa
changed the title
|
|
Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project). why reproductions are required |
|
@karam-mustafa read this: #10809 |
here is my basic service class |
|
I'll say it one more time. Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project). why reproductions are requiredOtherwise, please use our Discord channel (Support). We are using GitHub to track Bug Reports, Feature Requests, and Regressions. |
|
|
Your reproduction doesn't start. |
|
I just tested your repro (after minor changes to make it minimal as possible while valid). Your repro doesn't reproduces the issue. full code// main.ts
import { NestFactory } from '@nestjs/core';
import type { NestExpressApplication } from '@nestjs/platform-express';
import { AppModule } from './app.module';
async function bootstrap() {
const app = await NestFactory.create<NestExpressApplication>(AppModule);
await app.listen(process.env.PORT || 3000);
}
bootstrap();import { Module, Controller, Get, Inject, Injectable, Scope } from '@nestjs/common';
import { REQUEST } from '@nestjs/core';
import { Request } from 'express';
@Injectable({ scope: Scope.REQUEST })
class Foo {
constructor(
@Inject(REQUEST) public readonly request: Request,
) {
console.log('====================================');
console.log(typeof request === 'undefined'); // false
console.log('====================================');
}
}
class AppService extends Foo {
constructor(
@Inject(REQUEST) public readonly request: Request,
) {
super(request)
console.log(typeof request === 'undefined'); // false
}
}
@Controller()
class AppController {
constructor(private readonly appService: AppService) {}
@Get()
getHello(): string {
return 'ok'
}
}
@Module({
imports: [],
controllers: [AppController],
providers: [AppService],
})
export class AppModule {} |
Sorry for this confusion, I will let you know again when the repo could reproduce the issue, thanks for your patience |
|
Having the same issue:
Any tips on how I can help you refine this issue further from this point? |
|
@micalevisk so I'm guessing this might be related to this PR #10809 (perhaps |
|
Same problem. Error when migrating from 9.2.1 to 9.3.1 - UnhandledPromiseRejectionWarning: TypeError: this.graphInspector.insertEntrypointDefinition is not a function |
|
@Durairaj that's another one. Upgrade your |
|
@kamilmysliwiec since no one shared any repro, I guess we should revert that PR for now. |
|
@micalevisk in the 9.2.1 edition neither MiddlewareModule::bindHandler or MiddlewareModule::registerHandler function is being called (no breakpoints or console logs that I added seem to trigger in the transpiled js code of nestjs). These are the places your changes seem to reside. Im not in the position to share the repo. Will keep you posted if I have more info to share. |
|
@davidzwa another thing you could do: define the env var |
|
That tip is really nice! I think this extract might help you: Before the listen call we see |
|
great, thanks now could you please remove the nest/packages/core/injector/instance-wrapper.ts Lines 215 to 223 in d9c394b I'm suggesting this because that was another change introduced in 9.3.1 (PR #10698) |
|
Problem's gone with your suggestion. (FYI: my post above was 9.3.1 ofcourse) No more 'introspected as durable for the PrinterFileController nor PrinterController. |
|
@kamilmysliwiec ok so here we go |
|
Right, will look at it as well as soon as I get some free time. Disappointed my first contribution broke stuff... |
|
just reading through the thread, @davidzwa the example on CodeSandbox does it break when you run it locally? |
|
@vizio360 it doesn't we have no repro so far |
|
@davidzwa could you check the dependencies chain of Hopefully you don't have too many :) |
|
Any pipes with scope |
I still hope you agree that |
|
wondering if we could replace that with |
|
@karam-mustafa are you also using AutoMapper? |
|
@davidzwa I managed to force the error caused by Will create an issue, even though this is tested on nestjs 9.3.1 and we still don't know if the PRs that have been reverted can be reapplied. I think it will depend on what we understand about the issue with AutoMapper. |
@kamilmysliwiec I will try and get automapper in my repro repo, and see if I can figure out what the issue is. |
I didn't install it before |
|
@karam-mustafa |
|
@micalevisk note - I didn't revert #10698 yet |
|
@kamilmysliwiec @micalevisk just experienced the problem in our code base with nestjs 9.3.2 and we do not use AutoMapper. |
|
that's expected as your PR wasn't reverted yet (I thought it was :p). So now you have a reproduction of that bug that you can work with, right? |
|
Correct, and we are not using |
|
Just pitching in shortly, I was using 2 Crud mixins in my controller. It was too late in the night for me to eliminate that as a cause |
|
@kamilmysliwiec @micalevisk so in 9.3.2 11018 is out while 10698 is still included? |
|
@vizio360 exactly. 9.3.2 doesn't fix the bug introduced in 9.3.0 |
|
@davidzwa ok, can you reintroduce AutoMapper and make this change isDependencyTreeDurable(lookupRegistry = []) {
if (!(0, shared_utils_1.isUndefined)(this.isTreeDurable)) {
return this.isTreeDurable;
}
if (this.durable === true) {
this.isTreeDurable = true;
this.printIntrospectedAsDurable();
return this.isTreeDurable;
}
const isStatic = this.isDependencyTreeStatic();
if (isStatic) {
return false;
}
const isTreeNonDurable = this.introspectDepsAttribute((collection, registry) => collection.some((item) => !item.isDependencyTreeStatic() &&
!item.isDependencyTreeDurable(registry)), lookupRegistry);
this.isTreeDurable = !isTreeNonDurable && this.durable; //<--HERE
if (this.isTreeDurable) {
this.printIntrospectedAsDurable();
}
return this.isTreeDurable;
} |
|
I think I found the possible problem, at least in our code base it seems to be with pipes as they are marked as Adding our controller: @Get('/ourRoute')
async ourRoute(
@Request() req,
@Query('additionalData', AdditionalDataPipe) additionalData?: AdditionalDataEnum[]
): Promise<GetCurrentUserResponse> {The fixed Pipe: @Injectable({
scope: Scope.REQUEST,
})
export class AdditionalDataPipe implements PipeTransform<string, AdditionalDataEnum[]> {So, not sure if I need to guard against Pipes and treat them differently, I think I need to dig deeper. |
|
and I think I got a test reproducing the issue describe.only('when wrapper is non static and dep is static', () => {
it('should return false', () => {
const wrapper = new InstanceWrapper({ scope: Scope.REQUEST });
wrapper.addCtorMetadata(0, new InstanceWrapper());
expect(wrapper.isDependencyTreeDurable()).to.be.false;
});
});this fails. It should return false. |
|
possible fix in the PR above |
micalevisk
added
type: bug 😭
and removed
needs triage
|
nope more investigation needed... |
|
ok got more understanding of the issue, I started commenting on the PR itself. |
|
So I think I got to the bottom of it, and the PR #11041 has been updated. I've also added a diagram to explain the problem. |
|
Let's track this here #11041 |
Is there an existing issue for this?
Current behavior
I have a simple constrictor for a service class as following
constructor( @Inject(REQUEST) public readonly request: Request, runAuthDependincies: Boolean = true, )after you had released v9.3.1 this request is undefined
Minimum reproduction code
https://codesandbox.io/s/withered-monad-zgg1nu
Steps to reproduce
just updating the packages to 9.3.1
Expected behavior
The request property should not be undefined as before
Package
@nestjs/common@nestjs/core@nestjs/microservices@nestjs/platform-express@nestjs/platform-fastify@nestjs/platform-socket.io@nestjs/platform-ws@nestjs/testing@nestjs/websocketsOther package
No response
NestJS version
9.3.1
Packages versions
Node.js version
16.3
In which operating systems have you tested?
Other
No response
The text was updated successfully, but these errors were encountered: