fix: the loading order issue in multi-module mode #324
Conversation
WalkthroughThe change updates the nested loop logic in Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
core/aop-runtime/test/aop-runtime.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. core/aop-runtime/test/fixtures/mutli/a/A.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. core/aop-runtime/test/fixtures/mutli/b/B.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/aop-runtime/test/aop-runtime.test.ts (3)
262-262: Fix typo in test suite name.The test suite name contains a typo: "mutil aop" should be "multi aop".
- describe('mutil aop', () => { + describe('multi aop', () => {
266-269: Fix typos in fixture directory paths.The fixture paths contain typos: "mutli" should be "multi" in all directory references.
- path.join(__dirname, 'fixtures/mutli/a'), - path.join(__dirname, 'fixtures/mutli/b'), - path.join(__dirname, 'fixtures/mutli/c'), - path.join(__dirname, 'fixtures/mutli/cross'), + path.join(__dirname, 'fixtures/multi/a'), + path.join(__dirname, 'fixtures/multi/b'), + path.join(__dirname, 'fixtures/multi/c'), + path.join(__dirname, 'fixtures/multi/cross'),
263-281: Add cleanup logic for consistent test practices.This test suite lacks cleanup logic (afterEach) that other test suites in this file implement. For consistency and to prevent potential resource leaks, consider adding module cleanup.
describe('multi aop', () => { + let modules: Array<LoadUnitInstance>; + it('should work', async () => { - const modules = await CoreTestHelper.prepareModules([ + modules = await CoreTestHelper.prepareModules([ // ... paths ]); // ... assertions }); + + afterEach(async () => { + if (modules) { + for (const module of modules) { + await LoadUnitFactory.destroyLoadUnit(module.loadUnit); + await LoadUnitInstanceFactory.destroyLoadUnitInstance(module); + } + } + }); });core/aop-runtime/test/fixtures/mutli/cross/Cross.ts (1)
21-34: Document the purpose of empty advice method implementations.All advice methods are empty implementations. While this might be intentional for testing the AOP framework itself, consider adding comments to clarify the purpose.
export class BaseAdvice implements IAdvice<Base> { + // Empty implementations for testing multi-module AOP loading async beforeCall(_ctx: AdviceContext<Base>): Promise<void> { + // Intentionally empty for test purposes } async afterReturn(_ctx: AdviceContext<Base>, _result: any): Promise<void> { + // Intentionally empty for test purposes } async afterThrow(_ctx: AdviceContext<Base>, _error: Error): Promise<void> { + // Intentionally empty for test purposes } async afterFinally(_ctx: AdviceContext<Base>): Promise<void> { + // Intentionally empty for test purposes } async around(_ctx: AdviceContext<Base>, _next: () => Promise<any>): Promise<any> { + // Intentionally empty for test purposes } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/aop-runtime/test/aop-runtime.test.ts(1 hunks)core/aop-runtime/test/fixtures/mutli/a/A.ts(1 hunks)core/aop-runtime/test/fixtures/mutli/a/package.json(1 hunks)core/aop-runtime/test/fixtures/mutli/b/B.ts(1 hunks)core/aop-runtime/test/fixtures/mutli/b/package.json(1 hunks)core/aop-runtime/test/fixtures/mutli/c/Base.ts(1 hunks)core/aop-runtime/test/fixtures/mutli/c/package.json(1 hunks)core/aop-runtime/test/fixtures/mutli/cross/Cross.ts(1 hunks)core/aop-runtime/test/fixtures/mutli/cross/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- core/aop-runtime/test/fixtures/mutli/c/package.json
- core/aop-runtime/test/fixtures/mutli/cross/package.json
- core/aop-runtime/test/fixtures/mutli/a/package.json
- core/aop-runtime/test/fixtures/mutli/b/package.json
- core/aop-runtime/test/fixtures/mutli/b/B.ts
- core/aop-runtime/test/fixtures/mutli/c/Base.ts
- core/aop-runtime/test/fixtures/mutli/a/A.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
core/aop-runtime/test/aop-runtime.test.ts (1)
274-280: Verify the expected module loading order.The test validates specific module IDs and their order. Ensure this expected order is correct and reflects the actual module dependency resolution logic.
The test expects modules to load in this specific order. Please verify this matches the intended module loading behavior for the multi-module AOP scenario.
core/aop-runtime/test/fixtures/mutli/cross/Cross.ts (2)
11-18: LGTM! Proper AOP configuration.The crosscut configuration correctly targets the
Baseclass and itshellomethod usingPointcutType.CLASS. The advice is properly configured with public access level.
4-4:✅ Verification successful
Verify the Base class import path consistency.
The import references
../c/Basewhich should align with the directory structure. Ensure this path is correct after fixing the "mutli" typo in the directory structure.
🏁 Script executed:
#!/bin/bash # Description: Verify the Base class exists at the expected location and check directory structure consistency # Check if Base.ts exists in the expected location fd -t f "Base.ts" core/aop-runtime/test/fixtures/ --exec echo "Found Base.ts at: {}" # Check the actual directory structure under fixtures fd -t d -d 2 "." core/aop-runtime/test/fixtures/ --exec echo "Directory: {}"Length of output: 860
Import path is consistent with directory structure
The import in core/aop-runtime/test/fixtures/mutli/cross/Cross.ts
import { Base } from '../c/Base';correctly resolves to core/aop-runtime/test/fixtures/mutli/c/Base.ts per the current layout. No changes required.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Bug Fixes
New Features