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: extend support Class #276

Merged
merged 1 commit into from
Dec 19, 2024
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
15 changes: 15 additions & 0 deletions example/middleware/hello.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { MiddlewareFunc } from '../../src/index.js';

export const hello: MiddlewareFunc = async (ctx, next) => {
console.log('Hello middleware');
console.log(ctx.app.BaseContextClass);
console.log(ctx.app.Service);
console.log(ctx.service);
console.log(ctx.app.timing);
console.log(ctx.app.lifecycle);
console.log(ctx.request.ctx.app.timing);
console.log(ctx.request.app.timing);
console.log(ctx.request.response.app.timing);
console.log(ctx.response.request.app.timing);
Comment on lines +4 to +13
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove or replace console.log statements with proper logging

Multiple console.log statements appear to be for debugging purposes. This could impact performance in production and pollute logs.

Consider:

  1. Using a proper logging framework
  2. Adding appropriate log levels
  3. Removing debug statements before production
-  console.log('Hello middleware');
-  console.log(ctx.app.BaseContextClass);
-  console.log(ctx.app.Service);
-  console.log(ctx.service);
-  console.log(ctx.app.timing);
-  console.log(ctx.app.lifecycle);
-  console.log(ctx.request.ctx.app.timing);
-  console.log(ctx.request.app.timing);
-  console.log(ctx.request.response.app.timing);
-  console.log(ctx.response.request.app.timing);
+  ctx.logger.debug('Hello middleware executed', {
+    timing: ctx.app.timing,
+    lifecycle: ctx.app.lifecycle
+  });

Committable suggestion skipped: line range outside the PR's diff.

await next();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling around next() call

The middleware should handle potential errors from downstream middleware.

-  await next();
+  try {
+    await next();
+  } catch (err) {
+    ctx.logger.error('Error in hello middleware chain', err);
+    throw err;
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await next();
try {
await next();
} catch (err) {
ctx.logger.error('Error in hello middleware chain', err);
throw err;
}

};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
},
"homepage": "https://github.com/eggjs/egg-core#readme",
"dependencies": {
"@eggjs/koa": "^2.19.2",
"@eggjs/koa": "^2.20.1",
"@eggjs/router": "^3.0.5",
"@eggjs/utils": "^4.0.2",
"egg-logger": "^3.5.0",
Expand Down
6 changes: 3 additions & 3 deletions src/base_context_class.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
import type { EggCore, EggCoreContext } from './egg.js';
import type { EggCore, ContextDelegation } from './egg.js';

/**
* BaseContextClass is a base class that can be extended,
* it's instantiated in context level,
* {@link Helper}, {@link Service} is extending it.
*/
export class BaseContextClass {
ctx: EggCoreContext;
ctx: ContextDelegation;
app: EggCore;
config: Record<string, any>;
service: BaseContextClass;

/**
* @since 1.0.0
*/
constructor(ctx: EggCoreContext) {
constructor(ctx: ContextDelegation) {
/**
* @member {Context} BaseContextClass#ctx
* @since 1.0.0
Expand Down
51 changes: 43 additions & 8 deletions src/egg.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
/* eslint-disable prefer-spread */
import assert from 'node:assert';
import { debuglog } from 'node:util';
import { Application as KoaApplication } from '@eggjs/koa';
import type { ContextDelegation, MiddlewareFunc } from '@eggjs/koa';
import {
Application as KoaApplication, Context as KoaContext,
Request as KoaRequest, Response as KoaResponse,
} from '@eggjs/koa';
import type {
ContextDelegation as KoaContextDelegation,
MiddlewareFunc as KoaMiddlewareFunc,
Next,
} from '@eggjs/koa';
import { EggConsoleLogger } from 'egg-logger';
import { RegisterOptions, ResourcesController, EggRouter as Router } from '@eggjs/router';
import type { ReadyFunctionArg } from 'get-ready';
Expand All @@ -15,7 +22,7 @@ import utils from './utils/index.js';

const debug = debuglog('@eggjs/core:egg');

const EGG_LOADER = Symbol.for('egg#loader');
export const EGG_LOADER = Symbol.for('egg#loader');

export interface EggCoreOptions {
baseDir: string;
Expand All @@ -27,12 +34,40 @@ export interface EggCoreOptions {

export type EggCoreInitOptions = Partial<EggCoreOptions>;

export type { ContextDelegation, MiddlewareFunc, Next } from '@eggjs/koa';
// export @eggjs/koa classes
export {
KoaRequest, KoaResponse, KoaContext, KoaApplication,
};

// export @eggjs/koa types
export type {
Next, KoaMiddlewareFunc, KoaContextDelegation,
};

// export @eggjs/core classes
export class Request extends KoaRequest {
declare app: EggCore;
declare response: Response;
declare ctx: ContextDelegation;
}

export class Response extends KoaResponse {
declare app: EggCore;
declare request: Request;
declare ctx: ContextDelegation;
}

export interface EggCoreContext extends ContextDelegation {
app: EggCore;
export class Context extends KoaContext {
declare app: EggCore;
declare request: Request;
declare response: Response;
declare service: BaseContextClass;
}

// export @eggjs/core types
export type ContextDelegation = KoaContextDelegation & Context;
export type MiddlewareFunc<T extends ContextDelegation = ContextDelegation> = KoaMiddlewareFunc<T>;

export class EggCore extends KoaApplication {
options: EggCoreOptions;
timing: Timing;
Expand Down Expand Up @@ -159,7 +194,7 @@ export class EggCore extends KoaApplication {
use(fn: MiddlewareFunc) {
assert(typeof fn === 'function', 'app.use() requires a function');
debug('[use] add middleware: %o', fn._name || fn.name || '-');
this.middleware.push(fn);
this.middleware.push(fn as unknown as KoaMiddlewareFunc);
return this;
}

Expand Down Expand Up @@ -229,7 +264,7 @@ export class EggCore extends KoaApplication {
*
* @see https://eggjs.org/en/advanced/loader.html#beforestart
*
* @param {Function|AsyncFunction} scope function will execute before app start
* @param {Function} scope function will execute before app start
* @param {string} [name] scope name, default is empty string
*/
beforeStart(scope: Fun, name?: string) {
Expand Down
2 changes: 1 addition & 1 deletion src/loader/context_loader.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import assert from 'node:assert';
import { type ContextDelegation } from '@eggjs/koa';
import { isClass, isPrimitive } from 'is-type-of';
import { FileLoader, EXPORTS, type FileLoaderOptions } from './file_loader.js';
import type { ContextDelegation } from '../egg.js';

const CLASS_LOADER = Symbol('classLoader');

Expand Down
21 changes: 13 additions & 8 deletions src/loader/egg_loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import { ContextLoader, ContextLoaderOptions } from './context_loader.js';
import utils, { Fun } from '../utils/index.js';
import sequencify from '../utils/sequencify.js';
import { Timing } from '../utils/timing.js';
import type { EggCoreContext, EggCore, MiddlewareFunc } from '../egg.js';
import type { ContextDelegation, EggCore, MiddlewareFunc } from '../egg.js';
import { BaseContextClass } from '../base_context_class.js';

const debug = debuglog('@eggjs/core:egg_loader');
const debug = debuglog('@eggjs/core/loader/egg_loader');

const originalPrototypes: Record<string, any> = {
request: Request.prototype,
Expand Down Expand Up @@ -1064,7 +1064,7 @@ export class EggLoader {
for (const rawFilepath of filepaths) {
const filepath = this.resolveModule(rawFilepath)!;
if (!filepath) {
debug('loadExtend %o not found', rawFilepath);
// debug('loadExtend %o not found', rawFilepath);
continue;
}
if (filepath.endsWith('/index.js')) {
Expand All @@ -1073,9 +1073,14 @@ export class EggLoader {
this.app.deprecate(`app/extend/${name}/index.ts is deprecated, use app/extend/${name}.ts instead`);
}

const ext = await this.requireFile(filepath);
let ext = await this.requireFile(filepath);
// if extend object is Class, should use Class.prototype instead
if (isClass(ext)) {
ext = ext.prototype;
}
const properties = Object.getOwnPropertyNames(ext)
.concat(Object.getOwnPropertySymbols(ext) as any[]);
.concat(Object.getOwnPropertySymbols(ext) as any[])
.filter(name => name !== 'constructor'); // ignore class constructor for extend

for (const property of properties) {
if (mergeRecord.has(property)) {
Expand Down Expand Up @@ -1108,7 +1113,7 @@ export class EggLoader {
Object.defineProperty(proto, property, descriptor!);
mergeRecord.set(property, filepath);
}
debug('merge %j to %s from %s', Object.keys(ext), name, filepath);
debug('merge %j to %s from %s', properties, name, filepath);
}
this.timing.end(`Load extend/${name}.js`);
}
Expand Down Expand Up @@ -1682,7 +1687,7 @@ function wrapControllerClass(Controller: typeof BaseContextClass, fullPath: stri
}

function controllerMethodToMiddleware(Controller: typeof BaseContextClass, key: string) {
return function classControllerMiddleware(this: EggCoreContext, ...args: any[]) {
return function classControllerMiddleware(this: ContextDelegation, ...args: any[]) {
const controller: any = new Controller(this);
if (!this.app.config.controller?.supportParams) {
args = [ this ];
Expand Down Expand Up @@ -1718,7 +1723,7 @@ function wrapObject(obj: Record<string, any>, fullPath: string, prefix?: string)
}

function objectFunctionToMiddleware(func: Fun) {
async function objectControllerMiddleware(this: EggCoreContext, ...args: any[]) {
async function objectControllerMiddleware(this: ContextDelegation, ...args: any[]) {
if (!this.app.config.controller?.supportParams) {
args = [ this ];
}
Expand Down
13 changes: 13 additions & 0 deletions test/fixtures/extend-with-class/app/controller/home.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export default async function() {
const status = Number(this.query.status || 200);
this.status = status;
this.etag = '2.2.2.2';
Comment on lines +1 to +4
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation and error handling

The status code parsing lacks validation and error handling.

Add proper validation:

 export default async function() {
-  const status = Number(this.query.status || 200);
+  const rawStatus = this.query.status;
+  const status = rawStatus ? parseInt(rawStatus, 10) : 200;
+  if (isNaN(status) || status < 100 || status > 599) {
+    this.throw(400, `Invalid status code: ${rawStatus}`);
+  }
   this.status = status;
   this.etag = '2.2.2.2';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export default async function() {
const status = Number(this.query.status || 200);
this.status = status;
this.etag = '2.2.2.2';
export default async function() {
const rawStatus = this.query.status;
const status = rawStatus ? parseInt(rawStatus, 10) : 200;
if (isNaN(status) || status < 100 || status > 599) {
this.throw(400, `Invalid status code: ${rawStatus}`);
}
this.status = status;
this.etag = '2.2.2.2';

this.body = {
returnAppContext: this.appContext,
returnAppRequest: this.request.appRequest,
returnAppResponse: this.response.appResponse,
returnAppApplication: this.app.appApplication,
status: this.status,
etag: this.etag,
};
};
7 changes: 7 additions & 0 deletions test/fixtures/extend-with-class/app/extend/application.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { EggCore } from '../../../../../src/index.js'

export default class Application extends EggCore {
get appApplication() {
return 'app application';
}
};
11 changes: 11 additions & 0 deletions test/fixtures/extend-with-class/app/extend/context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Context } from '../../../../../src/index.js'

export default class MyContext extends Context {
get appContext() {
return this.app ? 'app context' : 'no app context';
}

ajax() {
return 'app ajax patch';
}
}
7 changes: 7 additions & 0 deletions test/fixtures/extend-with-class/app/extend/request.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { Request } from '../../../../../src/index.js'

export default class AppRequest extends Request {
get appRequest() {
return this.response.app.timing ? 'app request' : 'no app request';
}
}
17 changes: 17 additions & 0 deletions test/fixtures/extend-with-class/app/extend/response.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Response } from '../../../../../src/index.js'

export default class AppResponse extends Response {
get appResponse() {
return this.app.timing ? 'app response' : 'no app response';
}

set status(code) {
this._explicitStatus = true;
this.res.statusCode = code;
this.res.statusMessage = 'http status code ' + code;
}
Comment on lines +8 to +12
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add status code validation

The status setter directly modifies response properties without validating the status code range.

Add validation to ensure the status code is within valid HTTP range (100-599):

 set status(code) {
+  if (code < 100 || code > 599) {
+    throw new Error(`Invalid status code: ${code}`);
+  }
   this._explicitStatus = true;
   this.res.statusCode = code;
   this.res.statusMessage = 'http status code ' + code;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
set status(code) {
this._explicitStatus = true;
this.res.statusCode = code;
this.res.statusMessage = 'http status code ' + code;
}
set status(code) {
if (code < 100 || code > 599) {
throw new Error(`Invalid status code: ${code}`);
}
this._explicitStatus = true;
this.res.statusCode = code;
this.res.statusMessage = 'http status code ' + code;
}


get etag() {
return 'etag ok';
}
}
3 changes: 3 additions & 0 deletions test/fixtures/extend-with-class/app/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default (app) => {
app.get('/', app.controller.home);
};
4 changes: 4 additions & 0 deletions test/fixtures/extend-with-class/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "extend-with-class",
"type": "module"
}
38 changes: 38 additions & 0 deletions test/loader/mixin/load_extend_class.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { strict as assert } from 'node:assert';
import request from 'supertest';
import mm from 'mm';
import { Application, createApp } from '../../helper.js';

describe('test/loader/mixin/load_extend_class.test.ts', () => {
let app: Application;
before(async () => {
app = createApp('extend-with-class');
await app.loader.loadPlugin();
await app.loader.loadConfig();
await app.loader.loadRequestExtend();
await app.loader.loadResponseExtend();
await app.loader.loadApplicationExtend();
await app.loader.loadContextExtend();
await app.loader.loadController();
await app.loader.loadRouter();
await app.loader.loadMiddleware();
});
after(() => app.close());
afterEach(mm.restore);

it('should load app.context app.request app.response', () => {
assert((app as any).appApplication);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid type assertion with as any

Using type assertions reduces type safety.

-    assert((app as any).appApplication);
+    interface ExtendedApp extends Application {
+      appApplication: unknown;
+    }
+    assert((app as ExtendedApp).appApplication);

Committable suggestion skipped: line range outside the PR's diff.


return request(app.callback())
.get('/')
.expect({
returnAppContext: 'app context',
returnAppRequest: 'app request',
returnAppResponse: 'app response',
returnAppApplication: 'app application',
status: 200,
etag: 'etag ok',
})
.expect(200);
});
});
Loading