-
-
Notifications
You must be signed in to change notification settings - Fork 8k
fix(testing): auto-init fastify adapter for middleware registration #15385
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
fix(testing): auto-init fastify adapter for middleware registration #15385
Conversation
Pull Request Test Coverage Report for Build 481bc0ec-33a8-4162-8bf2-c5199bf1f5faDetails
💛 - Coveralls |
packages/testing/testing-module.ts
Outdated
if (typeof (httpAdapter as any)?.init === 'function') { | ||
const originalInit = (proxy as any).init; | ||
(proxy as any).init = async function (this: any) { | ||
await (httpAdapter as any).init(); | ||
if (originalInit) { | ||
return originalInit.call(this); | ||
} | ||
return this; | ||
}; | ||
} | ||
|
||
return proxy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to a dedicated method? (patchAppProxyInit?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted to patchAppProxyInit
method as requested!
@kamilmysliwiec I've extracted the logic to patchAppProxyInit method as requested. As the issue reporter @yawhide also noted, the fundamental problem is that:
The current implementation only wraps the proxy's init() method, which means the adapter still isn't initialized when app.use() is called, resulting in the same error. I apologize for not catching this limitation earlier. The tests are indeed failing with the same error. Given that making createNestApplication() async would be a breaking change (as the issue reporter mentioned), perhaps the best approach is to:
OR, as an alternative approach without breaking changes:
This would allow users to write cleaner test code:
What do you think? Should I update the PR with this approach instead? |
What about modifying the fastify adapter's |
66c6803
to
8b1dd91
Compare
This change doesn't enable using The core issue is that Fastify's middleware registration is inherently asynchronous (requires @fastify/middie plugin initialization), while Express allows synchronous middleware registration. To enable
If either of these issues (or both) need to be addressed, I'm happy to create a separate issue to discuss further. |
8b1dd91
to
d602f74
Compare
// Fastify requires @fastify/middie plugin to be registered before middleware can be used. | ||
// Unlike Express, middleware registration in Fastify must happen after initialization. | ||
// We provide a helpful error message to guide developers to call app.init() first. | ||
if (!this.isMiddieRegistered) { | ||
Logger.warn( | ||
'Middleware registration requires the "@fastify/middie" plugin to be registered first. ' + | ||
'Make sure to call app.init() before registering middleware with the Fastify adapter. ' + | ||
'See https://github.com/nestjs/nest/issues/15310 for more details.', | ||
FastifyAdapter.name, | ||
); | ||
throw new TypeError('this.instance.use is not a function'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory we could also lazily register middleware in case middie is not yet registered
example:
- if
use()
is called and middie is not registered - add the middleware to the internal "middleware" cache (preserve the registration order)
- once middie is registered (upon the "init()" call), check if that cache is not empty and if so, flush it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! Implemented the lazy middleware registration approach as you recommended.
5bc7a23
to
07291c2
Compare
Thank you @mag123c! LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #15310
When using the FastifyAdapter in NestJS testing scenarios, calling app.use() to register middleware before app.init() throws the following error:
TypeError: this.instance.use is not a function
This occurs because FastifyAdapter requires the @fastify/middie plugin to be registered before middleware can be used, but this registration only happens during the adapter's init() method. In testing environments, users can call app.use() immediately after creating the application but before app.init(), leading to this runtime error.
What is the new behavior?
The TestingModule now automatically calls adapter.init() when creating a NestApplication if the adapter has an init method.
This ensures that adapters like FastifyAdapter are properly initialized before being used.
Does this PR introduce a breaking change?