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

feat: support cjs and esm both by tshy #9

Merged
merged 1 commit into from
Feb 4, 2025
Merged

feat: support cjs and esm both by tshy #9

merged 1 commit into from
Feb 4, 2025

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Feb 4, 2025

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

Summary by CodeRabbit

  • New Features
    • Introduced a robust tracer functionality that generates unique trace IDs for improved application tracing.
  • Documentation
    • Rebranded the package to "@eggjs/tracer" with updated installation instructions, usage examples, and a new contributors section.
  • Refactor
    • Streamlined internal architecture and module integration for enhanced performance and clearer TypeScript support.
  • Chores
    • Revamped dependency management and build workflows, ensuring compatibility with Node.js ≥ 18.19.0.

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257
Copy link

coderabbitai bot commented Feb 4, 2025

Walkthrough

The changes update multiple configuration and workflow files while overhauling the tracer implementation. ESLint settings have been modified, GitHub templates and CI workflows have been adjusted or replaced, and the package has been rebranded from "egg‑tracer" to "@eggjs/tracer". Legacy re-export and configuration files for tracer have been removed. In their place, new classes and TypeScript modules (e.g., TracerBoot, TracerApplication, TracerContext, and Tracer) have been introduced along with updated tests and a strict TS configuration.

Changes

Files Change Summary
.eslintignore, .eslintrc Updated lint configuration: new ignore entries (test/fixtures, __snapshots__), removal of node_modules; extended ESLint config now includes TypeScript and node prefix rules.
.github/ISSUE_TEMPLATE.md, .github/PULL_REQUEST_TEMPLATE.md, .github/workflows/ci.yml, .github/workflows/nodejs.yml, .github/workflows/release.yml Removed issue/PR templates and old CI workflow; added a new multi-OS/node version workflow; updated release workflow to use a new repository and removed manual triggers.
.gitignore, CHANGELOG.md, History.md, README.md Added new ignore patterns; updated changelog with new version entries; removed legacy history file; rebranded package and updated badges, installation instructions, and contributor section in README.
agent.js, app.js, app/extend/agent.js, app/extend/application.js, app/extend/context.js Removed legacy modules that re-exported or extended tracer functionality.
config/config.default.js, index.d.ts, index.js, lib/index.js, lib/tracer.js, index.test-d.ts Removed legacy tracer configuration, type declarations, and readiness logic.
package.json Rebranded the package; added publishConfig and exports; updated main, types, files; overhauled dependencies and scripts; set Node engine requirements and module type to ES modules.
src/agent.ts, src/app.ts, src/app/extend/agent.ts, src/app/extend/application.ts, src/app/extend/context.ts, src/boot.ts, src/config/config.default.ts, src/index.ts, src/lib/tracer.ts, src/typings/index.d.ts Introduced new tracer classes and interfaces (TracerBoot, TracerApplication, TracerContext, and Tracer) with updated app integration and configuration using TypeScript.
Test files (test/error.tracer.test.ts, test/fixtures/apps/error-tracer-test/agent.js, test/index.test-d.ts, test/index.test.ts, test/plugin.test.ts, test/tracer.test.ts, and deleted JS versions) Transitioned tests to TypeScript, refactored tracer tests, updated type assertions, and removed redundant test files.
tsconfig.json Added new TypeScript configuration with strict settings, ES2022 target, and NodeNext module resolution.

Sequence Diagram(s)

sequenceDiagram
    participant App as EggCore
    participant Boot as TracerBoot
    participant TA as TracerApplication
    participant TC as TracerContext
    participant Tr as Tracer
    participant Client as User Request

    %% Boot sequence
    App->>Boot: Invoke didLoad()
    Boot-->>App: Set app[isReady] = true

    %% Application tracer initialization flow
    Client->>App: Incoming request
    App->>TA: Request tracer instance (getter)
    alt Tracer not cached
        TA->>Tr: Create a new Tracer instance using config
        Tr-->>TA: Return tracer instance with traceId
    else Use cached tracer
        TA-->>TA: Return existing tracer instance
    end

    %% Context tracer access
    App->>TC: Resolve tracer in request context
    alt Not yet initialized
        TC->>Tr: Instantiate Tracer from config
        Tr-->>TC: Return new tracer with unique traceId
    else
        TC-->>TC: Return cached tracer instance
    end
    TC-->>App: Provide traceId for response
Loading

Poem

Hop, hop, hop in lines of code,
New tracers bloom along the road.
Bugs retreat as tests align,
In Egg.js fields the changes shine.
A bunny’s cheer, so light and free—
🐇 CodeRabbit's joy for all to see!
Hop on, little code, hop on!

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@fengmk2 fengmk2 merged commit dba5b9c into master Feb 4, 2025
12 of 13 checks passed
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🔭 Outside diff range comments (1)
README.md (1)

42-54: Custom Tracer Example – Variable Mutation Issue

In the example for building a custom tracer, the counter is declared as a constant on line 46 (const counter = 0;) but is later incremented using counter++ on line 50. Since constants cannot be reassigned, this will throw an error at runtime.
Proposed Fix:

- const counter = 0;
+ let counter = 0;
🧹 Nitpick comments (7)
src/app/extend/application.ts (1)

7-21: Consider adding JSDoc comments for better documentation.

While the implementation is correct, adding JSDoc comments would improve code documentation and provide better IDE support.

Add documentation like this:

+/**
+ * Application class with tracer support.
+ * @extends EggCore
+ */
 export default class TracerApplication extends EggCore {
+  /** Cached tracer instance before application is ready */
   [cacheTracer]: Tracer | undefined;

+  /**
+   * Get tracer instance.
+   * @returns {Tracer} New tracer instance if app is ready, cached instance otherwise
+   */
   get tracer(): Tracer {
test/error.tracer.test.ts (2)

17-23: Add test descriptions for better clarity.

Consider adding descriptions for what each assertion is testing.

   it('should get app, agent tracer', () => {
+    // Verify tracer count before ready state
     assert.equal(app.appBeforeReadyTracers.length, 3);
     assert.equal(app.agent.agentBeforeReadyTracers.length, 3);

+    // Verify tracer count after ready state
     assert.equal(app.appAfterReadyTracers.length, 3);
     assert.equal(app.agent.agentAfterReadyTracers.length, 3);
   });

25-31: Consider adding more test cases for error scenarios.

The current test only covers the happy path. Consider adding tests for error scenarios.

Add tests for:

  • Invalid trace IDs
  • Missing trace IDs
  • Error responses
.github/workflows/release.yml (1)

7-12: External Workflow Reference Update

The job now references eggjs/github-actions/.github/workflows/node-release.yml@master which reflects the updated workflow source. This aligns with the project’s rebranding and updated CI/CD process.
Suggestion: It might be worthwhile to reference a specific version tag instead of master to ensure build stability over time.

.github/workflows/nodejs.yml (1)

9-18: Job Definition and External Workflow Usage

The job named under the key Job delegates to an external workflow at
node-modules/github-actions/.github/workflows/node-test.yml@master. While the external usage is correct, consider renaming the job key from the generic Job to a more descriptive identifier (e.g., nodeTest) for increased clarity in logs and debugging.

CHANGELOG.md (2)

20-24: Review Changelog Separator and Version Header Formatting
The insertion of a horizontal rule ("---") and the subsequent version header ("1.1.0 / 2017-09-07") appear unconventional. This style can trigger markdownlint issues (e.g., MD003 for heading style) and may affect readability. Consider using a consistent ATX heading style and a clear separator that aligns with the rest of the document.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

24-24: Heading style
Expected: atx; Actual: setext

(MD003, heading-style)


25-37: Improve Historical Entries Formatting
The historical section uses bold text (e.g., "features", "others") as stand-ins for headings and shows indentation inconsistencies (e.g., list indentations flagged by MD007 and MD036). For better clarity and to adhere to markdownlint guidelines, convert these to proper ATX headings and adjust the list indentations accordingly.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

27-27: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


28-28: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


30-30: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


31-31: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


33-33: Heading style
Expected: atx; Actual: setext

(MD003, heading-style)


36-36: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0fa496 and 202bb16.

📒 Files selected for processing (43)
  • .eslintignore (1 hunks)
  • .eslintrc (1 hunks)
  • .github/ISSUE_TEMPLATE.md (0 hunks)
  • .github/PULL_REQUEST_TEMPLATE.md (0 hunks)
  • .github/workflows/ci.yml (0 hunks)
  • .github/workflows/nodejs.yml (1 hunks)
  • .github/workflows/release.yml (1 hunks)
  • .gitignore (1 hunks)
  • CHANGELOG.md (1 hunks)
  • History.md (0 hunks)
  • README.md (3 hunks)
  • agent.js (0 hunks)
  • app.js (0 hunks)
  • app/extend/agent.js (0 hunks)
  • app/extend/application.js (0 hunks)
  • app/extend/context.js (0 hunks)
  • config/config.default.js (0 hunks)
  • index.d.ts (0 hunks)
  • index.js (0 hunks)
  • index.test-d.ts (0 hunks)
  • lib/index.js (0 hunks)
  • lib/tracer.js (0 hunks)
  • package.json (2 hunks)
  • src/agent.ts (1 hunks)
  • src/app.ts (1 hunks)
  • src/app/extend/agent.ts (1 hunks)
  • src/app/extend/application.ts (1 hunks)
  • src/app/extend/context.ts (1 hunks)
  • src/boot.ts (1 hunks)
  • src/config/config.default.ts (1 hunks)
  • src/index.ts (1 hunks)
  • src/lib/tracer.ts (1 hunks)
  • src/typings/index.d.ts (1 hunks)
  • test/error.tracer.test.js (0 hunks)
  • test/error.tracer.test.ts (1 hunks)
  • test/fixtures/apps/error-tracer-test/agent.js (0 hunks)
  • test/index.test-d.ts (1 hunks)
  • test/index.test.js (0 hunks)
  • test/index.test.ts (1 hunks)
  • test/plugin.test.ts (2 hunks)
  • test/tracer.test.js (0 hunks)
  • test/tracer.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
💤 Files with no reviewable changes (19)
  • lib/tracer.js
  • test/index.test.js
  • app/extend/context.js
  • test/fixtures/apps/error-tracer-test/agent.js
  • index.test-d.ts
  • test/tracer.test.js
  • app.js
  • index.d.ts
  • app/extend/application.js
  • index.js
  • .github/PULL_REQUEST_TEMPLATE.md
  • .github/ISSUE_TEMPLATE.md
  • config/config.default.js
  • app/extend/agent.js
  • agent.js
  • .github/workflows/ci.yml
  • History.md
  • lib/index.js
  • test/error.tracer.test.js
✅ Files skipped from review due to trivial changes (6)
  • .eslintignore
  • src/agent.ts
  • src/typings/index.d.ts
  • src/app.ts
  • .gitignore
  • src/app/extend/agent.ts
🧰 Additional context used
🪛 LanguageTool
README.md

[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 740 characters long)
Context: ...r [![NPM version][npm-image]][npm-url] [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] Node.js Version [![PRs Welcome](https://img.shields.io/bad...

(EN_EXCESSIVE_EXCLAMATION)

🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md

24-24: Heading style
Expected: atx; Actual: setext

(MD003, heading-style)


27-27: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


28-28: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


30-30: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


31-31: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


33-33: Heading style
Expected: atx; Actual: setext

(MD003, heading-style)


36-36: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Node.js / Test (windows-latest, 22)
  • GitHub Check: Node.js / Test (windows-latest, 20)
🔇 Additional comments (35)
src/index.ts (1)

1-8: LGTM! Clean modular structure with proper ESM support.

The file follows ESM best practices with explicit .js extensions and maintains a clean separation of concerns.

src/boot.ts (1)

4-10: LGTM! Clean implementation of ILifecycleBoot.

The class follows good practices with proper encapsulation and async lifecycle management.

src/lib/tracer.ts (1)

4-18: LGTM! Clean implementation with modern JavaScript features.

The class effectively uses private fields and lazy initialization for efficient tracing.

Let's verify the compatibility of the crypto module with the new Node.js version requirement:

✅ Verification successful

Crypto Compatibility Verified

The implementation in src/lib/tracer.ts is clean and leverages modern JavaScript features appropriately. We verified that the crypto.randomUUID() function is available in Node.js starting from v14.17.0. Since our project requires Node.js v18.19.0 or later, the usage of randomUUID() is fully compatible.

No changes are needed here.

🏁 Scripts executed

The following scripts were executed for the analysis:

Web query:

What Node.js version introduced the crypto.randomUUID() function?

Length of output: 14733

src/config/config.default.ts (3)

1-1: LGTM! Import statement follows ESM conventions.

The .js extension in the import path is correctly specified, which is required for ESM imports.


3-16: LGTM! Well-documented interface with type-safe configuration.

The interface is properly documented with JSDoc comments, and the configuration correctly implements the interface using type assertion.


18-22: LGTM! Module declaration properly extends EggAppConfig.

The module declaration correctly extends the @eggjs/core module to include the tracer configuration.

test/index.test-d.ts (4)

1-3: LGTM! Import statements are correctly specified.

The imports follow ESM conventions and include all necessary types.


5-7: LGTM! Type assertions are properly set up.

Empty objects are correctly cast to their respective types for testing purposes.


9-11: LGTM! Context type assertions are comprehensive.

Type assertions correctly verify both string types for traceId properties and Tracer type for the tracer property.


13-16: LGTM! Application and agent type assertions are comprehensive.

Type assertions correctly verify both string types for traceId properties and Tracer types for tracer properties.

test/plugin.test.ts (3)

1-1: LGTM! Import statement follows ESM conventions.

Required types and utilities are correctly imported using ESM syntax.


4-4: LGTM! Type annotation is properly specified.

The app variable is correctly typed as MockApplication.


19-19: LGTM! Regex pattern is more precise.

The regex pattern is now properly anchored and correctly validates the UUID format.

src/app/extend/context.ts (3)

1-3: LGTM! Imports and symbol declaration are properly specified.

The imports follow ESM conventions, and the symbol is correctly declared for private storage.


5-6: LGTM! Class declaration and property are well-defined.

The class properly extends Context, and the TRACER property is correctly typed.


20-25: LGTM! Module declaration properly extends Context interface.

The interface is correctly extended with properly typed tracer-related properties.

src/app/extend/application.ts (2)

1-5: LGTM! Clean imports and well-defined symbols.

The imports are appropriate, and the symbols are well-named for their purpose.


23-27: LGTM! Clean type declaration.

The module augmentation for EggCore is correctly implemented.

test/error.tracer.test.ts (1)

1-16: LGTM! Well-structured test setup.

The test setup follows best practices with proper lifecycle hooks and cleanup.

test/tracer.test.ts (1)

1-17: LGTM! Proper test setup.

The test setup is well-structured with appropriate imports and lifecycle hooks.

.eslintrc (1)

2-5: LGTM! Appropriate ESLint configuration for TypeScript.

The configuration correctly extends TypeScript-specific rules and enforces node prefix.

tsconfig.json (1)

1-10: TypeScript Configuration Validation

The configuration correctly extends @eggjs/tsconfig and enables strict type checking with options such as "strict": true and "noImplicitAny": true. Setting "target": "ES2022" along with "module" and "moduleResolution" set to "NodeNext" is appropriate for the modern ECMAScript features expected in this project.

.github/workflows/nodejs.yml (1)

1-8: CI Workflow Trigger Configuration

The workflow is correctly set to trigger on pushes and pull requests to the master branch. Overall, the event configuration looks clear and maintains consistency with project standards.

README.md (4)

1-10: Rebranding and Badge Updates

The title and badges have been updated to reflect the new package name @eggjs/tracer, including the addition of badges for Node.js version and pull request reviews. This rebranding is consistent throughout the file.

🧰 Tools
🪛 LanguageTool

[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 740 characters long)
Context: ...r [![NPM version][npm-image]][npm-url] [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] Node.js Version [![PRs Welcome](https://img.shields.io/bad...

(EN_EXCESSIVE_EXCLAMATION)


24-26: Installation Instruction Update

The installation instructions now correctly use npm i @eggjs/tracer. This update is clear and consistent with the rebranding.


32-37: Plugin Configuration Example

The example configuration in the plugin file now specifies the package as @eggjs/tracer, which is correct. This helps users set up the tracer plugin with the correct package reference.


73-78: Contributors Section Enhancement

The new contributors section, with a badge linking to the contributors graph, is a valuable addition to showcase community involvement.

package.json (7)

2-6: Rebranding and Publish Configuration

Changing the package name to @eggjs/tracer along with the addition of "publishConfig": { "access": "public" } is appropriately aligned with the project’s rebranding and public distribution goals.


8-15: eggPlugin Export Settings

The eggPlugin section now includes an exports object that clearly defines paths for "import", "require", and "typescript". This supports proper resolution for different module systems and enhances type safety.


35-37: Engine Version Enforcement

Requiring Node.js version >= 18.19.0 ensures that users are on supported versions following the breaking change. It’s essential to also confirm that the documentation and CI workflows reflect this requirement.


61-70: Updated Build and Test Scripts

The expanded scripts section—for linting, testing, cleaning, and prepublish routines—reflects a modernized development process. Please verify that commands like tshy, tshy-after, and attw --pack execute successfully in CI.


71-77: Module System Declaration and tshy Exports

Specifying "type": "module" and the additional tshy exports solidify the package’s ES module support while still catering to CommonJS via dual exports. This configuration boosts compatibility and clarity.


78-90: Dual Export Configuration

The root-level exports clearly differentiates between "import" and "require" paths, including correct paths for both default JavaScript files and TypeScript declaration files. This is a crucial update for consumers using different module systems.


91-98: Files, Main, and Module Fields

The files array and updates to "types", "main", and "module" fields ensure that only the necessary directories are included in the package and that consumers reference the correct build artifacts. These settings are now in line with the revised project structure.

CHANGELOG.md (1)

13-16: Clarify Node.js Support Version
There is an inconsistency between the PR objectives and the changelog entry. The PR summary indicates that support for Node.js versions earlier than 18.19.0 is dropped, while the changelog entry on line 15 currently states "* Drop Node.js < 14 support". Please update the changelog to accurately reflect the intended breaking change.

🧰 Tools
🪛 LanguageTool

[grammar] ~15-~15: After the number ‘14’, use a plural noun. Did you mean “supports”?
Context: ...⚠ BREAKING CHANGES * Drop Node.js < 14 support ### Features * upgrade all deps to la...

(CD_NNU)

Comment on lines +4 to +8
describe('test/index.test.ts', () => {
it('should work with lib', () => {
assert.equal(typeof Tracer, 'function');
});
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage for the Tracer class.

The current test only verifies that Tracer is a function. Consider adding tests for:

  • traceId generation and uniqueness
  • context handling
  • traceId caching behavior

Example test cases:

it('should generate unique traceIds', () => {
  const ctx = {} as Context;
  const tracer1 = new Tracer(ctx);
  const tracer2 = new Tracer(ctx);
  assert.notEqual(tracer1.traceId, tracer2.traceId);
});

it('should cache traceId', () => {
  const ctx = {} as Context;
  const tracer = new Tracer(ctx);
  const id1 = tracer.traceId;
  const id2 = tracer.traceId;
  assert.equal(id1, id2);
});

it('should store context', () => {
  const ctx = { foo: 'bar' } as Context;
  const tracer = new Tracer(ctx);
  assert.equal(tracer.ctx, ctx);
});

Comment on lines +8 to +18
get tracer(): Tracer {
if (!this[TRACER]) {
this[TRACER] = new this.app.config.tracer.Class(this);
}
return this[TRACER];
}

get traceId(): string {
return this.tracer.traceId;
}
}
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 for tracer initialization.

While the getters are well-implemented, the tracer initialization could fail if the config is invalid.

Add error handling:

 get tracer(): Tracer {
   if (!this[TRACER]) {
+    if (!this.app.config.tracer?.Class) {
+      throw new Error('Tracer Class is not configured');
+    }
     this[TRACER] = new this.app.config.tracer.Class(this);
   }
   return this[TRACER];
 }
📝 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
get tracer(): Tracer {
if (!this[TRACER]) {
this[TRACER] = new this.app.config.tracer.Class(this);
}
return this[TRACER];
}
get traceId(): string {
return this.tracer.traceId;
}
}
get tracer(): Tracer {
if (!this[TRACER]) {
if (!this.app.config.tracer?.Class) {
throw new Error('Tracer Class is not configured');
}
this[TRACER] = new this.app.config.tracer.Class(this);
}
return this[TRACER];
}
get traceId(): string {
return this.tracer.traceId;
}
}

Comment on lines +18 to +83
it('should get app, agent tracer', () => {
assert(app[isReady]);
assert(app.agent[isReady]);

let [ appTracer_1, appTracer_2, appTracer_3 ] = app.appBeforeReadyTracers;
let [ agentTracer_1, agentTracer_2, agentTracer_3 ] = app.agent.agentBeforeReadyTracers;

assert.equal(appTracer_1, appTracer_2);
assert.equal(appTracer_1, appTracer_3);

assert(appTracer_1.traceId);
assert(appTracer_2.traceId);
assert(appTracer_3.traceId);

assert.equal(appTracer_1, appTracer_2);
assert.equal(appTracer_1, appTracer_3);

assert.equal(appTracer_1.traceId, appTracer_2.traceId);
assert.equal(appTracer_1.traceId, appTracer_3.traceId);

assert(agentTracer_1.traceId);
assert(agentTracer_2.traceId);
assert(agentTracer_3.traceId);

assert.equal(agentTracer_1, agentTracer_2);
assert.equal(agentTracer_1, agentTracer_3);

assert(agentTracer_1.traceId);
assert(agentTracer_2.traceId);
assert(agentTracer_3.traceId);

assert.equal(agentTracer_1.traceId, agentTracer_2.traceId);
assert.equal(agentTracer_1.traceId, agentTracer_3.traceId);

// app ready
[ appTracer_1, appTracer_2, appTracer_3 ] = app.appAfterReadyTracers;

assert.notEqual(appTracer_1, appTracer_2);
assert.notEqual(appTracer_1, appTracer_3);

assert(appTracer_1.traceId);
assert(appTracer_2.traceId);
assert(appTracer_3.traceId);

assert.notEqual(appTracer_1, appTracer_2);
assert.notEqual(appTracer_1, appTracer_3);

assert.notEqual(appTracer_1.traceId, appTracer_2.traceId);
assert.notEqual(appTracer_1.traceId, appTracer_3.traceId);

// agent ready
[ agentTracer_1, agentTracer_2, agentTracer_3 ] = app.agent.agentAfterReadyTracers;
assert(agentTracer_1.traceId);
assert(agentTracer_2.traceId);
assert(agentTracer_3.traceId);

assert.notEqual(agentTracer_1, agentTracer_2);
assert.notEqual(agentTracer_1, agentTracer_3);

assert(agentTracer_1.traceId);
assert(agentTracer_2.traceId);
assert(agentTracer_3.traceId);

assert.notEqual(agentTracer_1.traceId, agentTracer_2.traceId);
assert.notEqual(agentTracer_1.traceId, agentTracer_3.traceId);
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor test for better maintainability.

The test contains redundant assertions and could be split into smaller, focused test cases.

Consider splitting into:

  1. "should have same tracer before ready"
  2. "should have different tracers after ready"
  3. "should maintain unique trace IDs after ready"

Example refactor:

it('should have same tracer before ready', () => {
  const [ tracer1, tracer2, tracer3 ] = app.appBeforeReadyTracers;
  assert.equal(tracer1, tracer2);
  assert.equal(tracer1, tracer3);
  assert.equal(tracer1.traceId, tracer2.traceId);
  assert.equal(tracer1.traceId, tracer3.traceId);
});

it('should have different tracers after ready', () => {
  const [ tracer1, tracer2, tracer3 ] = app.appAfterReadyTracers;
  assert.notEqual(tracer1, tracer2);
  assert.notEqual(tracer1, tracer3);
  assert.notEqual(tracer1.traceId, tracer2.traceId);
  assert.notEqual(tracer1.traceId, tracer3.traceId);
});

@fengmk2 fengmk2 deleted the egg-v4 branch February 4, 2025 11:17
fengmk2 pushed a commit that referenced this pull request Feb 4, 2025
[skip ci]

## [3.0.0](v2.1.0...v3.0.0) (2025-02-04)

### ⚠ BREAKING CHANGES

* drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Introduced a robust tracer functionality that generates unique trace
IDs for improved application tracing.
- **Documentation**
- Rebranded the package to "@eggjs/tracer" with updated installation
instructions, usage examples, and a new contributors section.
- **Refactor**
- Streamlined internal architecture and module integration for enhanced
performance and clearer TypeScript support.
- **Chores**
- Revamped dependency management and build workflows, ensuring
compatibility with Node.js ≥ 18.19.0.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

### Features

* support cjs and esm both by tshy ([#9](#9)) ([dba5b9c](dba5b9c))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant