refactor: use a better way to enable plugin#5646
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a plugin factory API and defineConfig helpers, introduces skipMerge handling in the loader, re-exports several tegg modules from egg, refactors tracer and development plugins to use plugin factories, updates example app to inject Tracer, and adjusts tests/configs and vitest timeouts. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application / Config
participant PluginFactory as definePluginFactory
participant Loader as EggLoader
participant PluginMeta as PluginMeta
App->>PluginFactory: import tracerPlugin()/developmentPlugin()
PluginFactory->>PluginFactory: validate (name, path)
PluginFactory-->>App: return { name, path, enable, skipMerge: true, ... }
App->>Loader: Loader.loadPlugins(pluginMap)
Loader->>PluginMeta: read eggPluginConfig (eggPluginConfig = plugin entry)
alt eggPluginConfig.skipMerge is true
Loader->>Loader: skip mergePluginConfig
else
Loader->>Loader: mergePluginConfig(eggPluginConfig)
end
Loader-->>App: plugin initialized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the plugin enablement mechanism by introducing a function-based approach for configuring plugins, demonstrated with the @eggjs/tracer plugin. The changes also add convenience re-exports for tegg-related modules and exports additional types from the main egg package.
Key Changes:
- Introduces a
tracerPlugin()function that returns plugin configuration, eliminating the need for manual plugin path configuration - Adds re-export modules (aop.ts, ajv.ts, dal.ts, helper.ts, orm.ts, transaction.ts) for tegg functionality
- Exports additional types and utilities from
@eggjs/teggin the main index
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/tracer/src/index.ts | Adds default export function tracerPlugin() that returns plugin configuration with automatic path resolution |
| packages/egg/src/transaction.ts | New re-export module for @eggjs/tegg/transaction |
| packages/egg/src/orm.ts | New re-export module for @eggjs/tegg/orm |
| packages/egg/src/lib/types.plugin.ts | Comments out schedule types import |
| packages/egg/src/index.ts | Adds Config type alias and exports additional tegg utilities (ContextProto, BackgroundTaskHelper, etc.) |
| packages/egg/src/helper.ts | New re-export module for @eggjs/tegg/helper |
| packages/egg/src/dal.ts | New re-export module for @eggjs/tegg/dal |
| packages/egg/src/aop.ts | New re-export module for @eggjs/tegg/aop |
| packages/egg/src/ajv.ts | New re-export module for @eggjs/tegg/ajv |
| examples/helloworld-tegg/config/plugin.ts | Demonstrates new plugin configuration approach using tracerPlugin() |
| examples/helloworld-tegg/app/port/controller/SimpleController.ts | Updates getFoo method to use Context and log tracer information |
Deploying egg-v3 with
|
| Latest commit: |
aaaeece
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d3db947b.egg-v3.pages.dev |
| Branch Preview URL: | https://egg-tracer-plugin.egg-v3.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5646 +/- ##
==========================================
+ Coverage 87.51% 87.67% +0.15%
==========================================
Files 565 565
Lines 11006 11009 +3
Branches 1243 1243
==========================================
+ Hits 9632 9652 +20
+ Misses 1292 1276 -16
+ Partials 82 81 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
aaaeece
|
| Status: | 🚫 Deploy failed. |
fengmk2
force-pushed
the
egg-tracer-plugin
branch
from
54e009a to
575544f
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
plugins/tracer/test/fixtures/apps/error-tracer-test/config/plugin.js:1
- Using
importin a.jsfile requires the file to be ES module (with\"type\": \"module\"in package.json or.mjsextension). Consider renaming this file toplugin.mjsor using CommonJS syntax withrequire().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/egg/src/lib/types.plugin.ts (1)
3-3: Explanation needed for commented import.The commented-out import of '@eggjs/development/types' lacks explanation. Please add a comment clarifying why this import is disabled in this refactor.
🧹 Nitpick comments (5)
packages/egg/src/config/config.unittest.ts (1)
3-9: Drop redundant type assertion after defineConfigdefineConfig already returns PartialEggConfig; the trailing cast isn’t needed.
-export default defineConfig({ +export default defineConfig({ logger: { consoleLevel: 'WARN', // disable buffer for unittest buffer: false, }, -}) as PartialEggConfig; +});plugins/tracer/src/index.ts (1)
20-24: Good: use import.meta.dirname; optional: drop redundant castThe path fix looks right and resolves earlier dirname concerns. The
as EggDefinePluginFactoryis unnecessary because definePluginFactory already returns that type.export default definePluginFactory({ name: 'tracer', enable: true, path: import.meta.dirname, -}) as EggDefinePluginFactory; +});packages/core/src/loader/egg_loader.ts (1)
619-657: Type eggPluginConfig to avoid any-typed accessAnnotate the in-function
eggPluginConfigto a minimal shape; keeps property reads safe without broadany.- async #mergePluginConfig(plugin: EggPluginInfo): Promise<void> { - let pkg: any; - let eggPluginConfig: any; + async #mergePluginConfig(plugin: EggPluginInfo): Promise<void> { + type PkgEggPlugin = { + name?: string; + dependencies?: string[]; + optionalDependencies?: string[]; + env?: string[]; + strict?: boolean; + dep?: string[]; + }; + let pkg: any; + let eggPluginConfig: PkgEggPlugin | undefined; const pluginPackage = path.join(plugin.path as string, 'package.json'); if (await utils.existsPath(pluginPackage)) { pkg = await readJSON(pluginPackage); - eggPluginConfig = pkg.eggPlugin; + eggPluginConfig = pkg.eggPlugin as PkgEggPlugin | undefined;packages/egg/src/lib/define.ts (1)
32-44: API hygiene: document return types and rationale.Consider clarifying in JSDoc that
defineConfigFactoryis a no‑op wrapper for type inference and tooling, mirroringdefineConfig. This avoids future attempts to add side effects here.packages/egg/src/config/config.local.ts (1)
3-9: Drop redundant type assertions in config files.
defineConfigalready returnsPartialEggConfig, so the trailingas PartialEggConfigcast is unnecessary. For consistency, remove it from bothconfig.local.ts(line 9) andconfig.unittest.ts(line 9):-}) as PartialEggConfig; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (30)
examples/helloworld-tegg/app/port/controller/SimpleController.ts(4 hunks)examples/helloworld-tegg/config/plugin.ts(1 hunks)packages/core/src/loader/egg_loader.ts(4 hunks)packages/core/src/types.ts(1 hunks)packages/egg/src/ajv.ts(1 hunks)packages/egg/src/aop.ts(1 hunks)packages/egg/src/config/config.local.ts(1 hunks)packages/egg/src/config/config.unittest.ts(1 hunks)packages/egg/src/config/plugin.ts(2 hunks)packages/egg/src/dal.ts(1 hunks)packages/egg/src/helper.ts(1 hunks)packages/egg/src/index.ts(2 hunks)packages/egg/src/lib/define.ts(1 hunks)packages/egg/src/lib/types.plugin.ts(1 hunks)packages/egg/src/lib/types.ts(0 hunks)packages/egg/src/orm.ts(1 hunks)packages/egg/src/transaction.ts(1 hunks)plugins/development/package.json(0 hunks)plugins/development/src/index.ts(1 hunks)plugins/development/test/development-ts.test.ts(2 hunks)plugins/development/test/fixtures/development-ts/config/plugin.ts(1 hunks)plugins/development/test/override.test.ts(1 hunks)plugins/development/test/process_mode_single.test.ts(2 hunks)plugins/schedule/test/all.test.ts(1 hunks)plugins/tracer/README.md(1 hunks)plugins/tracer/package.json(0 hunks)plugins/tracer/src/index.ts(1 hunks)plugins/tracer/test/fixtures/apps/error-tracer-test/config/plugin.js(1 hunks)plugins/tracer/test/fixtures/apps/plugin-test/config/plugin.js(1 hunks)tegg/plugin/controller/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- plugins/tracer/package.json
- packages/egg/src/lib/types.ts
- plugins/development/package.json
🧰 Additional context used
📓 Path-based instructions (8)
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use TypeScript for all source files in packages (no JavaScript source files)
Files:
packages/egg/src/transaction.tspackages/egg/src/index.tspackages/core/src/types.tspackages/egg/src/orm.tspackages/egg/src/helper.tspackages/egg/src/config/config.unittest.tspackages/egg/src/lib/define.tspackages/egg/src/ajv.tspackages/egg/src/config/plugin.tspackages/egg/src/lib/types.plugin.tspackages/egg/src/config/config.local.tspackages/egg/src/dal.tspackages/core/src/loader/egg_loader.tspackages/egg/src/aop.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
packages/egg/src/transaction.tsexamples/helloworld-tegg/config/plugin.tspackages/egg/src/index.tsplugins/development/src/index.tsplugins/tracer/src/index.tspackages/core/src/types.tspackages/egg/src/orm.tspackages/egg/src/helper.tspackages/egg/src/config/config.unittest.tspackages/egg/src/lib/define.tsplugins/development/test/fixtures/development-ts/config/plugin.tspackages/egg/src/ajv.tsexamples/helloworld-tegg/app/port/controller/SimpleController.tsplugins/development/test/override.test.tsplugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tspackages/egg/src/config/plugin.tspackages/egg/src/lib/types.plugin.tspackages/egg/src/config/config.local.tsplugins/schedule/test/all.test.tstegg/plugin/controller/vitest.config.tspackages/egg/src/dal.tspackages/core/src/loader/egg_loader.tspackages/egg/src/aop.ts
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts: For isolatedDeclarations support, all exported functions/methods/getters must have explicit return type annotations
Avoid computed property names with symbols in class declarations (no get SYM)
Add explicit type annotations for class properties when needed (no inferred exported property types)
Exported symbols must use unique symbol type (e.g., export const X: unique symbol = Symbol('x'))
Files:
packages/egg/src/transaction.tspackages/egg/src/index.tsplugins/development/src/index.tsplugins/tracer/src/index.tspackages/core/src/types.tspackages/egg/src/orm.tspackages/egg/src/helper.tspackages/egg/src/config/config.unittest.tspackages/egg/src/lib/define.tsplugins/development/test/fixtures/development-ts/config/plugin.tspackages/egg/src/ajv.tsplugins/development/test/override.test.tsplugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tspackages/egg/src/config/plugin.tspackages/egg/src/lib/types.plugin.tspackages/egg/src/config/config.local.tsplugins/schedule/test/all.test.tspackages/egg/src/dal.tspackages/core/src/loader/egg_loader.tspackages/egg/src/aop.ts
**/test/fixtures/**
📄 CodeRabbit inference engine (AGENTS.md)
Put reusable test data under test/fixtures/
Files:
plugins/tracer/test/fixtures/apps/error-tracer-test/config/plugin.jsplugins/development/test/fixtures/development-ts/config/plugin.tsplugins/tracer/test/fixtures/apps/plugin-test/config/plugin.js
{packages/**/test/**,plugins/**/test/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Node.js assert for assertions in Vitest tests
Files:
plugins/tracer/test/fixtures/apps/error-tracer-test/config/plugin.jsplugins/development/test/fixtures/development-ts/config/plugin.tsplugins/tracer/test/fixtures/apps/plugin-test/config/plugin.jsplugins/development/test/override.test.tsplugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tsplugins/schedule/test/all.test.ts
packages/egg/src/config/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store default configurations and plugin configs under packages/egg/src/config/
Files:
packages/egg/src/config/config.unittest.tspackages/egg/src/config/plugin.tspackages/egg/src/config/config.local.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
Files:
plugins/development/test/override.test.tsplugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tsplugins/schedule/test/all.test.ts
{packages/!(cookies)/**/test/**/*.test.ts,plugins/**/test/**/*.test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest test files must follow the naming pattern test/**/*.test.ts
Files:
plugins/development/test/override.test.tsplugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tsplugins/schedule/test/all.test.ts
🧠 Learnings (11)
📚 Learning: 2025-10-23T17:37:37.549Z
Learnt from: CR
PR: eggjs/egg#0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-10-23T17:37:37.549Z
Learning: Applies to tegg/core/*/src/index.ts : Export the public API from src/index.ts in each core package
Applied to files:
packages/egg/src/index.tspackages/egg/src/helper.tspackages/egg/src/dal.tspackages/egg/src/aop.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to plugins/*/src/types.ts : All plugins must provide src/types.ts that augments module 'egg' (module augmentation, config types, interface extensions, .ts import extensions, documented properties)
Applied to files:
plugins/development/src/index.tsplugins/tracer/src/index.tspackages/core/src/types.tspackages/egg/src/lib/define.tsplugins/development/test/fixtures/development-ts/config/plugin.tspackages/egg/src/lib/types.plugin.tspackages/core/src/loader/egg_loader.ts
📚 Learning: 2025-10-23T17:37:37.549Z
Learnt from: CR
PR: eggjs/egg#0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-10-23T17:37:37.549Z
Learning: Use Egg.js >= 4.1.0
Applied to files:
packages/egg/src/helper.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**/tsdown.config.ts,plugins/**/tsdown.config.ts,tools/**/tsdown.config.ts} : Config file exports must use typed intermediate variables (tsdown): const config: UserConfig = defineConfig(...); export default config
Applied to files:
packages/egg/src/config/config.unittest.tspackages/egg/src/config/config.local.ts
📚 Learning: 2025-10-23T17:37:37.549Z
Learnt from: CR
PR: eggjs/egg#0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-10-23T17:37:37.549Z
Learning: Applies to tegg/plugin/*/package.json : Plugin packages must define eggPlugin in package.json with dependencies
Applied to files:
packages/egg/src/lib/define.tspackages/egg/src/config/plugin.tspackages/core/src/loader/egg_loader.ts
📚 Learning: 2025-10-23T17:37:37.549Z
Learnt from: CR
PR: eggjs/egg#0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-10-23T17:37:37.549Z
Learning: Applies to tegg/plugin/*/{test,tests}/**/*.ts : Add Vitest tests for plugins using eggjs/mock; tests will be discovered by the root vitest config
Applied to files:
plugins/development/test/development-ts.test.tsplugins/development/test/process_mode_single.test.tstegg/plugin/controller/vitest.config.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**/vitest.config.ts,plugins/**/vitest.config.ts} : Packages using Vitest must include a vitest.config.ts and import test functions from vitest
Applied to files:
plugins/development/test/development-ts.test.tstegg/plugin/controller/vitest.config.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**/test/**,plugins/**/test/**} : Use Node.js assert for assertions in Vitest tests
Applied to files:
plugins/development/test/development-ts.test.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Use import { describe, it } from 'vitest' in tests
Applied to files:
plugins/development/test/development-ts.test.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts : Add explicit type annotations for class properties when needed (no inferred exported property types)
Applied to files:
packages/egg/src/lib/types.plugin.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/!(cookies)/**/vitest.config.ts,plugins/**/vitest.config.ts} : Vitest config must export a typed config (e.g., const config: UserWorkspaceConfig = defineProject(...); export default config)
Applied to files:
tegg/plugin/controller/vitest.config.ts
🧬 Code graph analysis (8)
plugins/development/src/index.ts (1)
packages/egg/src/lib/define.ts (2)
definePluginFactory(90-101)EggDefinePluginFactory(72-72)
plugins/tracer/src/index.ts (1)
packages/egg/src/lib/define.ts (2)
definePluginFactory(90-101)EggDefinePluginFactory(72-72)
packages/egg/src/config/config.unittest.ts (1)
packages/egg/src/lib/define.ts (1)
defineConfig(28-30)
packages/egg/src/lib/define.ts (2)
packages/egg/src/lib/types.ts (3)
EggAppConfig(84-270)EggAppInfo(17-17)EggEnvType(29-29)packages/core/src/types.ts (2)
EggAppConfig(51-58)EggAppInfo(1-16)
plugins/development/test/fixtures/development-ts/config/plugin.ts (1)
packages/egg/src/lib/types.ts (1)
EggPlugin(289-303)
examples/helloworld-tegg/app/port/controller/SimpleController.ts (2)
packages/egg/src/index.ts (2)
Inject(132-132)HTTPContext(152-152)plugins/tracer/src/index.ts (1)
Tracer(7-7)
plugins/development/test/development-ts.test.ts (1)
plugins/development/test/utils.ts (1)
getFilepath(7-9)
packages/egg/src/config/config.local.ts (1)
packages/egg/src/lib/define.ts (1)
defineConfig(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test bin (windows-latest, 22, 1/3)
- GitHub Check: Test bin (windows-latest, 22, 0/3)
- GitHub Check: Test bin (ubuntu-latest, 22, 1/3)
- GitHub Check: typecheck
- GitHub Check: Test bin (ubuntu-latest, 22, 0/3)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (35)
tegg/plugin/controller/vitest.config.ts (1)
1-11: LGTM! Configuration follows established patterns.The Vitest configuration correctly follows the required pattern with typed config using
UserWorkspaceConfigand proper export structure. The 20-second timeouts are reasonable for plugin integration tests.Based on learnings.
plugins/schedule/test/all.test.ts (1)
8-9: Consider investigating the root cause of Windows timeout.The conditional skip is a pragmatic workaround for CI stability, but it reduces test coverage on Windows and may mask platform-specific issues.
Consider adding a follow-up task to investigate why cluster tests time out on Windows—potential causes include timing assumptions, file-system latency, or process spawning differences on the platform.
plugins/development/test/override.test.ts (1)
42-43: Consider investigating the root cause of Windows timeout.The selective skip preserves some Windows coverage (the
overrideDefaultsuite still runs), which is better than skipping all tests. However, the timeout issue suggests underlying platform-specific problems.Consider investigating why file-watching tests fail on Windows—common culprits include asynchronous file-system events, different polling intervals, or timing assumptions that don't hold across platforms. Note that line 55 has an additional
it.skipwithin the already-skipped suite, which may indicate known instability even beyond the platform issue.packages/egg/src/orm.ts (1)
1-1: LGTM!The barrel re-export follows the established pattern for exposing tegg modules through the egg package surface.
packages/egg/src/dal.ts (1)
1-1: LGTM!The barrel re-export follows the established pattern for exposing tegg modules through the egg package surface.
packages/egg/src/aop.ts (1)
1-1: LGTM!The barrel re-export follows the established pattern for exposing tegg modules through the egg package surface.
examples/helloworld-tegg/config/plugin.ts (1)
1-5: LGTM!This correctly demonstrates the new explicit plugin import pattern from the PR objectives. The plugin factory approach enables automatic TypeScript type injection without relying on code generation.
packages/egg/src/transaction.ts (1)
1-1: LGTM!The barrel re-export follows the established pattern for exposing tegg modules through the egg package surface.
packages/egg/src/ajv.ts (1)
1-1: LGTM!The barrel re-export follows the established pattern for exposing tegg modules through the egg package surface.
examples/helloworld-tegg/app/port/controller/SimpleController.ts (3)
1-1: LGTM!The imports correctly add assertion support and tracer integration needed for the validation logic below.
Also applies to: 17-17
28-29: LGTM!The tracer injection follows the standard dependency injection pattern and integrates the new tracer plugin.
65-70: Assertions validate tracer integration in example.The runtime assertions verify that the tracer is correctly injected and accessible through both the context and dependency injection. This is appropriate for an example application that demonstrates the tracer plugin functionality.
plugins/tracer/README.md (3)
29-36: LGTM! Clear TypeScript plugin usage example.The updated example demonstrates the new explicit plugin import pattern effectively, showing how to use the plugin factory with the spread operator.
38-51: LGTM! Proper TypeScript class export.The custom Tracer class example correctly demonstrates ES6 class syntax with proper TypeScript export patterns.
55-66: LGTM! Clear configuration example.The configuration example properly demonstrates the use of
defineConfigwith custom Tracer class integration.packages/egg/src/helper.ts (1)
1-1: Verify this change aligns with PR scope.This file re-exports helpers from
@eggjs/tegg/helper, which appears unrelated to the plugin factory refactor described in the PR objectives. While not problematic, consider whether this should be part of a separate PR focused on API surface expansion.Based on learnings.
packages/core/src/types.ts (1)
38-39: LGTM! Clear documentation for the new flag.The
skipMergeproperty is well-documented and aligns with the plugin factory pattern introduced in this PR. The flag enables plugins defined viadefinePluginFactoryto bypass automatic package.json config merging.plugins/development/test/process_mode_single.test.ts (2)
10-10: LGTM! Proper import of the plugin factory.The import statement correctly references the plugin factory from the source directory.
19-21: LGTM! Clean usage of the plugin factory pattern.The test now uses the new plugin factory pattern via spread operator, which is cleaner and more type-safe than the previous explicit configuration.
plugins/development/test/development-ts.test.ts (2)
6-6: LGTM! Proper import addition.The
expectimport from vitest is correctly added for the new test assertions.
24-43: LGTM! Comprehensive validation of plugin configuration.The new test validates that the development plugin is properly configured in the local environment by checking the agent_config.json output. The assertions correctly verify all key plugin properties (enable, env, name, dependencies, optionalDependencies).
plugins/development/test/fixtures/development-ts/config/plugin.ts (3)
1-1: LGTM! Proper type import.The EggPlugin type import provides type safety for the plugin configuration export.
3-3: LGTM! Correct relative import path.The import path correctly references the plugin factory from the source directory using proper relative path traversal.
5-9: LGTM! Proper usage of the plugin factory with type safety.The fixture correctly demonstrates the new plugin factory pattern with the spread operator and includes a type assertion for EggPlugin to ensure type safety.
packages/egg/src/config/plugin.ts (2)
1-1: LGTM! Proper package import.The import statement correctly references the @eggjs/development package to use the new plugin factory.
73-73: LGTM! Clean integration of the plugin factory.The development plugin configuration now uses the new factory pattern via spread operator, which provides better type safety and explicit plugin enablement. This change aligns perfectly with the PR objectives.
plugins/development/src/index.ts (4)
1-1: LGTM! Proper imports for plugin factory pattern.The imports correctly reference the plugin factory utilities from the egg package.
3-3: LGTM! Module augmentation import.The side-effect import ensures TypeScript module augmentation from types.ts is loaded, which is essential for type safety.
As per coding guidelines.
5-23: Excellent documentation with clear usage example.The JSDoc provides comprehensive documentation including:
- Clear description of the plugin's purpose and environment restrictions
- Complete usage example with import and configuration
- Detailed parameter documentation
- Version tag (@SInCE 4.1.0)
This makes it easy for users to adopt the new plugin pattern.
24-30: LGTM! Proper plugin factory implementation.The plugin factory is correctly defined using
definePluginFactorywith appropriate metadata:
name,enable,path,env, anddependenciesare all properly configuredimport.meta.dirnameprovides the correct plugin path in ESM- The type assertion ensures the return type matches
EggDefinePluginFactoryNote: The
skipMerge: trueflag (set internally bydefinePluginFactory) ensures this plugin's config bypasses package.json merging, which aligns with the explicit plugin enablement pattern.packages/core/src/loader/egg_loader.ts (2)
445-447: Respect plugin.skipMerge to bypass package.json mergeThis gating is correct and necessary for factory-defined plugins that deliberately avoid pkg-driven merges.
1702-1708: dep → dependencies compatibility helper looks goodSimple and safe mapping that preserves existing
dependenciesif already set.packages/egg/src/index.ts (3)
15-17: Expose define helpers on egg public APIMakes the new config/plugin factory ergonomics available to users.
18-41: Add Config type alias (EggAppConfig as Config)Useful ergonomic alias for DI/injection use cases; no conflicts anticipated.
179-188: Re-export additional tegg surface (ContextProto, BackgroundTaskHelper, buses, MetadataUtil)Consistent with broader tegg exposure; OK.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/egg/src/lib/define.ts (1)
50-103: Type mismatch:skipMergeproperty missing fromEggPluginMeta.The
definePluginFactoryimplementation addsskipMerge: trueat line 100, butEggPluginMeta(lines 50-63) doesn't declare this property, causing TypeScript compilation errors under strict mode. Additionally,EggPluginOptionsshould excludeskipMergefrom user overrides.This issue was previously identified. Apply the fix suggested in the earlier review:
export interface EggPluginMeta { /** the plugin name, it can be used in `dep` */ name: string; /** whether enabled */ enable: boolean; /** the directory of the plugin package */ path: string; /** the dependent plugins, you can use the plugin name */ dependencies?: string[]; /** the optional dependent plugins. */ optionalDependencies?: string[]; /** specify the serverEnv that only enable the plugin in it, default to all envs */ env?: EggEnvType[]; + /** internal: skip merging eggPlugin from package.json */ + skipMerge?: boolean; } - export type EggPluginOptions = PartialDeep<Omit<EggPluginMeta, 'name'>>; + export type EggPluginOptions = PartialDeep<Omit<EggPluginMeta, 'name' | 'skipMerge'>>; - export type EggPluginFactory = (options?: EggPluginOptions) => Record<string, EggPluginMeta>; + export type EggPluginFactory = + (options?: EggPluginOptions) => Record<string, EggPluginMeta & { skipMerge: true }>;
🧹 Nitpick comments (2)
plugins/schedule/test/schedule-plugin.test.ts (1)
8-9: Pragmatic workaround, but investigate the root cause.Skipping flaky tests on Windows is a reasonable short-term solution, but it reduces test coverage on that platform. The "Hook timed out in 20000ms" error suggests the
beforeAllhook (which spawns a cluster with 2 workers) may be hanging on Windows.Consider investigating:
- Whether the cluster initialization completes on Windows
- If Windows-specific timing or process handling is causing the timeout
- Increasing the hook timeout specifically for Windows as an alternative to skipping
- Ensuring proper cleanup in the
afterAllhookWould you like me to generate a verification script to check if there are similar cluster-based tests that work on Windows, or help investigate alternative solutions?
plugins/tracer/src/index.ts (1)
24-24: Remove redundant type assertion.The type assertion
as EggPluginFactoryis unnecessary sincedefinePluginFactoryalready returnsEggPluginFactory. This assertion may be masking the type mismatch issue indefine.ts(missingskipMergeinEggPluginMeta).Remove the type assertion once the type definitions in
define.tsare fixed:- }) as EggPluginFactory; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/egg/src/lib/define.ts(1 hunks)plugins/development/src/index.ts(1 hunks)plugins/logrotator/vitest.config.ts(1 hunks)plugins/schedule/test/schedule-plugin.test.ts(1 hunks)plugins/tracer/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/development/src/index.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
plugins/logrotator/vitest.config.tsplugins/tracer/src/index.tsplugins/schedule/test/schedule-plugin.test.tspackages/egg/src/lib/define.ts
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts: For isolatedDeclarations support, all exported functions/methods/getters must have explicit return type annotations
Avoid computed property names with symbols in class declarations (no get SYM)
Add explicit type annotations for class properties when needed (no inferred exported property types)
Exported symbols must use unique symbol type (e.g., export const X: unique symbol = Symbol('x'))
Files:
plugins/logrotator/vitest.config.tsplugins/tracer/src/index.tsplugins/schedule/test/schedule-plugin.test.tspackages/egg/src/lib/define.ts
{packages/!(cookies)/**/vitest.config.ts,plugins/**/vitest.config.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest config must export a typed config (e.g., const config: UserWorkspaceConfig = defineProject(...); export default config)
Files:
plugins/logrotator/vitest.config.ts
{packages/**/vitest.config.ts,plugins/**/vitest.config.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Packages using Vitest must include a vitest.config.ts and import test functions from vitest
Files:
plugins/logrotator/vitest.config.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
Files:
plugins/schedule/test/schedule-plugin.test.ts
{packages/!(cookies)/**/test/**/*.test.ts,plugins/**/test/**/*.test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest test files must follow the naming pattern test/**/*.test.ts
Files:
plugins/schedule/test/schedule-plugin.test.ts
{packages/**/test/**,plugins/**/test/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Node.js assert for assertions in Vitest tests
Files:
plugins/schedule/test/schedule-plugin.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use TypeScript for all source files in packages (no JavaScript source files)
Files:
packages/egg/src/lib/define.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: eggjs/egg#0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-10-23T17:37:37.549Z
Learning: Applies to tegg/plugin/*/{test,tests}/**/*.ts : Add Vitest tests for plugins using eggjs/mock; tests will be discovered by the root vitest config
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**/vitest.config.ts,plugins/**/vitest.config.ts} : Packages using Vitest must include a vitest.config.ts and import test functions from vitest
Applied to files:
plugins/logrotator/vitest.config.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/!(cookies)/**/vitest.config.ts,plugins/**/vitest.config.ts} : Vitest config must export a typed config (e.g., const config: UserWorkspaceConfig = defineProject(...); export default config)
Applied to files:
plugins/logrotator/vitest.config.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to plugins/*/src/types.ts : All plugins must provide src/types.ts that augments module 'egg' (module augmentation, config types, interface extensions, .ts import extensions, documented properties)
Applied to files:
plugins/tracer/src/index.tspackages/egg/src/lib/define.ts
📚 Learning: 2025-10-18T11:59:58.226Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T11:59:58.226Z
Learning: Applies to {packages/**,plugins/**,tools/!(egg-bin)/**}/**/*.ts : Add explicit type annotations for class properties when needed (no inferred exported property types)
Applied to files:
packages/egg/src/lib/define.ts
🧬 Code graph analysis (2)
plugins/tracer/src/index.ts (1)
packages/egg/src/lib/define.ts (2)
definePluginFactory(91-103)EggPluginFactory(73-73)
packages/egg/src/lib/define.ts (2)
packages/egg/src/lib/types.ts (3)
EggAppConfig(84-270)EggAppInfo(17-17)EggEnvType(29-29)packages/core/src/types.ts (2)
EggAppConfig(51-58)EggAppInfo(1-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Test (macos-latest, 24, 2/5)
- GitHub Check: Test (windows-latest, 24, 1/5)
- GitHub Check: Test (windows-latest, 24, 4/5)
- GitHub Check: Test (ubuntu-latest, 22, 3/5)
- GitHub Check: Test (windows-latest, 22, 3/5)
- GitHub Check: Test (macos-latest, 24, 1/5)
- GitHub Check: Test (windows-latest, 22, 4/5)
- GitHub Check: Test (ubuntu-latest, 24, 2/5)
- GitHub Check: Test (macos-latest, 24, 5/5)
- GitHub Check: Test (macos-latest, 24, 4/5)
- GitHub Check: Test (ubuntu-latest, 24, 3/5)
- GitHub Check: Test (macos-latest, 22, 4/5)
- GitHub Check: Test (ubuntu-latest, 24, 1/5)
- GitHub Check: Test (windows-latest, 22, 1/5)
- GitHub Check: Test (ubuntu-latest, 24, 5/5)
- GitHub Check: Test (ubuntu-latest, 22, 2/5)
- GitHub Check: Test bin (windows-latest, 22, 1/3)
- GitHub Check: Test bin (windows-latest, 22, 0/3)
- GitHub Check: Test bin (ubuntu-latest, 22, 0/3)
- GitHub Check: Test bin (ubuntu-latest, 22, 1/3)
🔇 Additional comments (1)
plugins/logrotator/vitest.config.ts (1)
6-7: LGTM! Reasonable timeout extension for plugin tests.The extended timeouts (20 seconds) are appropriate given the architectural changes in this PR involving plugin loading and initialization. The configuration follows Vitest patterns correctly.
| export default definePluginFactory({ | ||
| name: 'tracer', | ||
| enable: true, | ||
| path: import.meta.dirname, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify plugin path resolves to the correct directory.
import.meta.dirname resolves to the directory containing this file (plugins/tracer/src/), but the plugin root is typically the parent directory (plugins/tracer/) where package.json resides.
Verify that the loader correctly handles plugins with entry points in subdirectories, or update the path:
- path: import.meta.dirname,
+ path: path.join(import.meta.dirname, '..'),If the above is needed, add the import at the top:
import path from 'node:path';Run this script to confirm how the plugin path is used by the loader:
🏁 Script executed:
#!/bin/bash
# Verify how plugin.path is used in the loader
rg -n "plugin\.path|pluginInfo\.path" packages/core/src/loader -C3Length of output: 3428
🏁 Script executed:
#!/bin/bash
# Find and examine formatPluginPathFromPackageJSON implementation
rg -n "formatPluginPathFromPackageJSON" packages/core/src/loader -C5 -A10Length of output: 3453
🏁 Script executed:
#!/bin/bash
# Verify tracer package.json location
find plugins/tracer -name "package.json" -type fLength of output: 151
🏁 Script executed:
#!/bin/bash
# Check tracer package.json structure
cat plugins/tracer/package.jsonLength of output: 2340
Update plugin path to point to the plugin root directory.
The loader expects plugin.path to resolve to the directory containing package.json. Since import.meta.dirname resolves to plugins/tracer/src/, the loader will fail to find plugins/tracer/package.json and will skip plugin configuration loading.
Update to:
import path from 'node:path';- path: import.meta.dirname,
+ path: path.join(import.meta.dirname, '..'),🤖 Prompt for AI Agents
In plugins/tracer/src/index.ts around line 23, plugin.path is currently set to
import.meta.dirname which points to plugins/tracer/src/ instead of the plugin
root; update by importing the node path module (import path from 'node:path')
and set plugin.path to the parent directory of import.meta.dirname (e.g.,
path.resolve(import.meta.dirname, '..')) so it resolves to plugins/tracer/ where
package.json lives.
1 participant
close #5645
Summary by CodeRabbit
New Features
Refactor
Tests