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

fix(platform-http): update the adapter configuration to be aligned with DI convention #2999

Merged
merged 2 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/ISSUE_TEMPLATE/01_bug-report.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
name: "\U0001F41B Bug report"
description: Create a report to help us improve
title: "[BUG] Title"
title: "Title"
labels: [":bug: bug"]
type: bug
assignees: ["Romakita"]
body:
- type: markdown
Expand Down
1 change: 1 addition & 0 deletions .github/ISSUE_TEMPLATE/02_feature-request.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
name: "\U0001F680 Feature request"
description: Suggest an idea for this library
labels: [":sparkles: enhancement"]
type: feature
assignees: ["Romakita"]
body:
- type: markdown
Expand Down
38 changes: 0 additions & 38 deletions .github/ISSUE_TEMPLATE/bug-report-test.md

This file was deleted.

16 changes: 16 additions & 0 deletions docs/.vitepress/config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,14 @@ export default defineConfig({
text: "Platform API",
link: `/docs/platform-api`
},
{
text: "Platform Serverless",
link: `/docs/platform-serverless`
},
{
text: "Platform Serverless HTTP",
link: `/docs/platform-serverless-http`
},
{
text: "Command",
link: `/docs/command`
Expand Down Expand Up @@ -434,6 +442,14 @@ export default defineConfig({
text: "Platform API",
link: `/docs/platform-api`
},
{
text: "Platform Serverless",
link: `/docs/platform-serverless`
},
{
text: "Platform Serverless HTTP",
link: `/docs/platform-serverless-http`
},
{
text: "Command",
link: `/docs/command`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,6 @@ declare global {
export class PlatformExpress extends PlatformAdapter<Express.Application> {
readonly NAME = "express";

readonly providers = [
{
token: PlatformHandler,
useClass: PlatformExpressHandler
},
{token: PlatformResponse},
{token: PlatformRequest},
{token: PlatformApplication},
{token: Platform}
];

#multer: typeof multer;

constructor() {
Expand Down Expand Up @@ -286,4 +275,9 @@ export class PlatformExpress extends PlatformAdapter<Express.Application> {
}
}

adapter(PlatformExpress);
adapter(PlatformExpress, [
{
token: PlatformHandler,
useClass: PlatformExpressHandler
}
]);
6 changes: 3 additions & 3 deletions packages/platform/platform-express/vitest.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ export default defineConfig(
coverage: {
...presets.test.coverage,
thresholds: {
statements: 96.63,
statements: 96.61,
branches: 95,
functions: 100,
lines: 96.63
lines: 96.61
}
}
}
}
);
);
27 changes: 23 additions & 4 deletions packages/platform/platform-http/src/common/fn/adapter.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,39 @@
import {Type} from "@tsed/core";
import {refValue} from "@tsed/di";
import {GlobalProviders, injector, ProviderOpts, refValue} from "@tsed/di";

import type {PlatformAdapter} from "../services/PlatformAdapter.js";
import {PlatformAdapter} from "../services/PlatformAdapter.js";
Comment on lines +2 to +4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Use consistent ESM extension
The import statements with ".js" extension are valid for ESM usage. Just ensure your bundler or environment supports it consistently across the platform.


const ADAPTER = "platform.adapter";

let globalAdapter: Type<PlatformAdapter<any>>;

/**
* Set the platform adapter
* Set the platform adapter token and his dependencies
*/
export function adapter(adapter?: Type<PlatformAdapter<any>>): Type<PlatformAdapter<any>> {
export function adapter(adapter: Type<PlatformAdapter<any>>, imports?: ProviderOpts[]): Type<PlatformAdapter<any>>;
/**
* Get the platform adapter token
*/
export function adapter(): Type<PlatformAdapter<any>>;
export function adapter(adapter?: Type<PlatformAdapter<any>>, imports: ProviderOpts[] = []): Type<PlatformAdapter<any>> {
Comment on lines +11 to +18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Clarify function overload documentation
The JSDoc references “Set the platform adapter token and his dependencies,” but doesn’t address the overload that retrieves the adapter token. Consider updating or separating the doc blocks to reflect both usage scenarios.

const ref = refValue<Type<PlatformAdapter<any>>>(ADAPTER);

if (adapter) {
globalAdapter ||= adapter;

imports?.forEach(({token, useClass}) => {
const provider = GlobalProviders.get(token);
if (useClass && provider) {
provider.useClass = useClass;
injector().set(token, provider);
}
});

injector()
.addProvider(PlatformAdapter, {
useClass: adapter
})
.alias(PlatformAdapter, "PlatformAdapter");
Comment on lines +24 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Handle missing providers and possible concurrency side effects
When reassigning provider.useClass in the loop, ensure that GlobalProviders.get(token) is defined. Otherwise, a missing provider might cause runtime errors. Also verify if reassigning useClass could lead to concurrency or lifecycle issues if multiple calls to adapter() modify the same provider in parallel.

}

ref.value ||= globalAdapter;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import {catchAsyncError} from "@tsed/core";
import {inject} from "@tsed/di";
import {Exception} from "@tsed/exceptions";
import {MulterError} from "multer";

import {PlatformTest} from "../../testing/PlatformTest.js";
import {MulterOptions} from "../decorators/multer/multerOptions.js";
import {MultipartFile} from "../decorators/multer/multipartFile.js";
import {EndpointMetadata} from "../domain/EndpointMetadata.js";
import {PlatformAdapter} from "../services/PlatformAdapter.js";
import {PlatformApplication} from "../services/PlatformApplication.js";
import {PlatformMulterMiddleware} from "./PlatformMulterMiddleware.js";

Expand All @@ -19,20 +21,16 @@ async function build(options = {}) {
const multer = {
fields: vi.fn().mockReturnValue(multerMiddleware)
};
const app = {
multer: vi.fn().mockReturnValue(multer)
};

const middleware = await PlatformTest.invoke<PlatformMulterMiddleware>(PlatformMulterMiddleware, [
{
token: PlatformApplication,
use: app
}
]);
const middleware = inject(PlatformMulterMiddleware);
const ctx: any = PlatformTest.createRequestContext();
ctx.endpoint = EndpointMetadata.get(Test, "upload");

return {middleware, ctx, multer, app, multerMiddleware};
const adapter = inject(PlatformAdapter);

vi.spyOn(adapter, "multipart").mockReturnValue(multer as any);

return {middleware, ctx, multer, adapter, multerMiddleware};
}

describe("PlatformMulterMiddleware", () => {
Expand All @@ -45,24 +43,24 @@ describe("PlatformMulterMiddleware", () => {
);
afterEach(() => PlatformTest.reset());
it("should create middleware", async () => {
const {middleware, ctx, multer, app, multerMiddleware} = await build({});
const {middleware, ctx, multer, adapter, multerMiddleware} = await build({});

await middleware.use(ctx);

expect(app.multer).toHaveBeenCalledWith({
expect(adapter.multipart).toHaveBeenCalledWith({
dest: "/dest"
});
expect(multer.fields).toHaveBeenCalledWith([{maxCount: undefined, name: "file1"}]);
expect(multerMiddleware).toHaveBeenCalledWith(ctx.request.raw, ctx.response.raw);
});
it("should create middleware with storage", async () => {
const {middleware, ctx, multer, app, multerMiddleware} = await build({
const {middleware, ctx, multer, adapter, multerMiddleware} = await build({
storage: "storage"
});

await middleware.use(ctx);

expect(app.multer).toHaveBeenCalledWith({
expect(adapter.multipart).toHaveBeenCalledWith({
storage: "storage"
});
expect(multer.fields).toHaveBeenCalledWith([{maxCount: undefined, name: "file1"}]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {MulterError} from "multer";

import {PlatformMulterField, PlatformMulterSettings} from "../config/interfaces/PlatformMulterSettings.js";
import {PlatformContext} from "../domain/PlatformContext.js";
import {PlatformAdapter} from "../services/PlatformAdapter.js";
import {PlatformApplication} from "../services/PlatformApplication.js";

export interface MulterInputOptions {
Expand All @@ -24,8 +25,6 @@ export class MulterException extends BadRequest {
* @middleware
*/
export class PlatformMulterMiddleware implements MiddlewareMethods {
protected app = inject(PlatformApplication);

async use(@Context() ctx: PlatformContext) {
try {
const {fields, options = {}} = ctx.endpoint.get(PlatformMulterMiddleware);
Expand All @@ -39,7 +38,7 @@ export class PlatformMulterMiddleware implements MiddlewareMethods {
delete settings.dest;
}

const multer = this.app.multer(settings);
const multer = inject(PlatformAdapter).multipart(settings);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Refactor injection usage.
Using inject(PlatformAdapter).multipart(settings) is correct for dynamically retrieving the adapter. If this call happens frequently, consider storing a reference in the class to avoid repetitive injection.


if (multer) {
const middleware: any = multer.fields(this.getFields({fields}));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {IncomingMessage, ServerResponse} from "node:http";

import {Type} from "@tsed/core";
import {injectable, ProviderOpts} from "@tsed/di";
import {configuration, constant, inject, injectable} from "@tsed/di";
import {$on} from "@tsed/hooks";
import {PlatformHandlerMetadata, PlatformLayer} from "@tsed/platform-router";

import {PlatformMulter, PlatformMulterSettings} from "../config/interfaces/PlatformMulterSettings.js";
Expand All @@ -10,17 +11,25 @@ import {application} from "../fn/application.js";
import {createHttpServer} from "../utils/createHttpServer.js";
import {createHttpsServer} from "../utils/createHttpsServer.js";
import {CreateServerReturn} from "../utils/createServer.js";
import type {PlatformApplication} from "./PlatformApplication.js";
import {Platform} from "./Platform.js";
import {PlatformApplication} from "./PlatformApplication.js";
import {PlatformHandler} from "./PlatformHandler.js";

export abstract class PlatformAdapter<App = TsED.Application> {
abstract readonly NAME: string;
/**
* Load providers in top priority
*/
providers: ProviderOpts[] = [];

get app(): PlatformApplication<App> {
return application();
readonly app = inject<PlatformApplication<App>>(PlatformApplication);

constructor() {
const platformApp = inject(PlatformApplication);

const {app, callback} = this.createApp();
platformApp.rawApp = app as any;
platformApp.rawCallback = callback;

$on("$afterInvoke", PlatformAdapter, () => {
configuration().set("PLATFORM_NAME", constant("PLATFORM_NAME") || this.NAME);
});
Comment on lines +21 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Unify usage of injected references and confirm $afterInvoke usage
The constructor re-injects PlatformApplication instead of relying on this.app. You could reference this.app to avoid redundancy. Also verify that $afterInvoke is the desired lifecycle hook for finalizing the PLATFORM_NAME; depending on your application flow, you might need an earlier or later hook.

}

getServers(): CreateServerReturn[] {
Expand Down Expand Up @@ -87,4 +96,4 @@ export interface PlatformBuilderSettings<App = TsED.Application> extends Partial
adapter?: Type<PlatformAdapter<App>>;
}

injectable(PlatformAdapter).alias("PlatformAdapter");
injectable(PlatformAdapter).imports([PlatformApplication, Platform, PlatformHandler]).alias("PlatformAdapter");
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ async function getPlatformApp() {
]);
configuration().logger = {};
platformApp.rawApp = createDriver() as any;
vi.spyOn(platformApp, "multer").mockReturnValue({} as never);

return {platformApp, platformHandler};
}
Expand Down Expand Up @@ -63,17 +62,6 @@ describe("PlatformApplication", () => {
platformApp.statics("/", {root: "/root"});
});
});
describe("multer()", () => {
it("should call statics", async () => {
// GIVEN
const {platformApp} = await getPlatformApp();

vi.spyOn(console, "warn");

// WHEN
platformApp.multer({});
});
});

describe("callback()", () => {
it("should return the callback", async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import {IncomingMessage, ServerResponse} from "node:http";

import {inject, injectable, ProviderScope} from "@tsed/di";
import {injectable, ProviderScope} from "@tsed/di";
import {PlatformRouter} from "@tsed/platform-router";

import {PlatformMulterSettings} from "../config/interfaces/PlatformMulterSettings.js";
import {PlatformAdapter} from "./PlatformAdapter.js";

declare global {
namespace TsED {
// @ts-ignore
Expand All @@ -19,28 +16,14 @@ declare global {
* @platform
*/
export class PlatformApplication<App = TsED.Application> extends PlatformRouter {
adapter: PlatformAdapter<App> = inject(PlatformAdapter<App>);

rawApp: App;
rawCallback: () => any;

constructor() {
super();

const {app, callback} = this.adapter.createApp();

this.rawApp = app;
this.rawCallback = callback;
}
rawCallback(): any {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Empty method body may lead to unhandled callbacks.
Previously, rawCallback might have been populated by the adapter; now it is an empty method returning any. Verify if other parts of the platform expect a functional implementation. If so, consider either implementing a default callback or refactoring the call site.


getApp(): App {
return this.rawApp;
}

multer(options: PlatformMulterSettings) {
return this.adapter.multipart(options);
}

callback(): (req: IncomingMessage, res: ServerResponse) => any;
callback(req: IncomingMessage, res: ServerResponse): any;
callback(req?: IncomingMessage, res?: ServerResponse) {
Expand Down
Loading
Loading