Skip to content

fix(tegg): preserve load error caller path#5939

Open
killagu wants to merge 1 commit intonextfrom
agent/egg-dev/417ded38
Open

fix(tegg): preserve load error caller path#5939
killagu wants to merge 1 commit intonextfrom
agent/egg-dev/417ded38

Conversation

@killagu
Copy link
Copy Markdown
Contributor

@killagu killagu commented May 7, 2026

Summary

  • Preserve the original caller-provided file path when wrapping tegg dynamic import failures.
  • Add regression coverage for the Windows file URL fallback path.

Tests

  • pnpm exec vitest run tegg/core/loader/test/Loader.test.ts
  • pnpm -C tegg/core/loader run typecheck
  • pnpm exec oxlint --type-aware tegg/core/loader/src/LoaderUtil.ts tegg/core/loader/test/Loader.test.ts
  • pnpm exec oxfmt --check tegg/core/loader/src/LoaderUtil.ts tegg/core/loader/test/Loader.test.ts
  • git diff --check origin/next...HEAD

Follow-up for CodeRabbit thread on #5932.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed incorrect file paths in module-load error messages on Windows so failures now report the original caller path for clearer, more accurate debugging.
  • Tests

    • Added a unit test to validate Windows-specific error-wrapping behavior and prevent regressions.

Copilot AI review requested due to automatic review settings May 7, 2026 18:12
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d14694a2-2725-44b5-a929-e4d8836d0b0a

📥 Commits

Reviewing files that changed from the base of the PR and between 09214b3 and 444f6eb.

📒 Files selected for processing (2)
  • tegg/core/loader/src/LoaderUtil.ts
  • tegg/core/loader/test/Loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tegg/core/loader/src/LoaderUtil.ts

📝 Walkthrough

Walkthrough

Centralizes Windows detection in LoaderUtil.isWindowsPlatform, changes LoaderUtil.loadFile to wrap dynamic-import failures with the original caller file path (originalFilePath), and adds a unit test that validates the Windows-specific error-message behavior.

Changes

Windows File Path Preservation in Error Messages

Layer / File(s) Summary
Public getters / surface
tegg/core/loader/src/LoaderUtil.ts
No functional change to the extension getter.
Platform detection helper
tegg/core/loader/src/LoaderUtil.ts
Adds static isWindowsPlatform(): boolean to centralize Windows checks.
Async Import Error Handling
tegg/core/loader/src/LoaderUtil.ts
Replaces inline process.platform === 'win32' with LoaderUtil.isWindowsPlatform() and uses createLoadError(originalFilePath, e) when import(filePath) fails.
Tests
tegg/core/loader/test/Loader.test.ts
New test mocks isWindowsPlatform() as true, forces a missing-module import, and asserts the wrapped error message preserves the original missing file path prefix.

Possibly Related PRs

  • eggjs/egg#5932: Modifies the same loader error path to standardize load error wrapping while preserving original file paths, including Windows import handling.

Poem

🐰 A Windows path once lost in translation,
Now stands preserved through transformation!
When imports fail and errors chime,
The original path returns each time. ✨

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(tegg): preserve load error caller path' clearly and concisely summarizes the main change: preserving the original file path when constructing load errors.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/egg-dev/417ded38

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 7, 2026

Deploying egg with  Cloudflare Pages  Cloudflare Pages

Latest commit: 444f6eb
Status: ✅  Deploy successful!
Preview URL: https://bd00c39d.egg-cci.pages.dev
Branch Preview URL: https://agent-egg-dev-417ded38.egg-cci.pages.dev

View logs

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.19%. Comparing base (0ebb28e) to head (444f6eb).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #5939   +/-   ##
=======================================
  Coverage   85.19%   85.19%           
=======================================
  Files         668      668           
  Lines       19288    19290    +2     
  Branches     3784     3784           
=======================================
+ Hits        16432    16434    +2     
  Misses       2464     2464           
  Partials      392      392           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates LoaderUtil.ts to use originalFilePath instead of filePath when reporting load errors, ensuring consistent error messages across different environments. A new test case was added to verify this behavior on Windows. Feedback was provided regarding the method used to mock process.platform in the tests, suggesting a more robust approach like using a helper method or standard mocking tools to avoid potential fragility in different Node.js environments.

Comment thread tegg/core/loader/test/Loader.test.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes TEgg loader error wrapping so that, when LoaderUtil.loadFile() falls back to import() on Windows and converts the path to a file:// URL, the thrown “load failed” error message still references the original caller-provided path (not the transformed URL). It also adds a regression test to ensure this behavior.

Changes:

  • Use originalFilePath (caller path) instead of the potentially rewritten filePath when wrapping dynamic import() failures.
  • Add a regression test that simulates win32 and asserts the error message preserves the caller path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tegg/core/loader/src/LoaderUtil.ts Preserve caller-provided path in createLoadError(...) when dynamic import fails after Windows file:// conversion.
tegg/core/loader/test/Loader.test.ts Add regression coverage ensuring error messages keep the original path under the win32 fallback branch.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 7, 2026

Deploying egg-v3 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 444f6eb
Status: ✅  Deploy successful!
Preview URL: https://e4b12d26.egg-v3.pages.dev
Branch Preview URL: https://agent-egg-dev-417ded38.egg-v3.pages.dev

View logs

@killagu killagu force-pushed the agent/egg-dev/417ded38 branch from 09214b3 to 444f6eb Compare May 7, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants