Skip to content

πŸ› Add error callback to AMD require in component loader#361

Merged
brianmhunt merged 2 commits into
mainfrom
fix/amd-require-error-callback
Apr 27, 2026
Merged

πŸ› Add error callback to AMD require in component loader#361
brianmhunt merged 2 commits into
mainfrom
fix/amd-require-error-callback

Conversation

@phillipc
Copy link
Copy Markdown
Member

@phillipc phillipc commented Apr 22, 2026

Problem

The AMD require call in possiblyGetConfigFromAmd (packages/utils.component/src/loaders.ts) passed only a success callback β€” no error callback. When AMD module resolution fails, the error is silently swallowed, leaving the component load hanging with no diagnostic.

Fix

Add an explicit error callback (third argument) to the AMD require call that surfaces the failure through the existing errorCallback mechanism, producing a clear message like:

Component 'my-comp': Failed to load AMD module: some/module β€” Error details…

Changes

  • packages/utils.component/src/loaders.ts: Add error callback to the (window.amdRequire || window.require) call.

Verification

  • All 106 existing utils.component tests pass.
  • No API surface change β€” purely an additive error path.

Summary by CodeRabbit

  • Bug Fixes
    • Improved module loading error messages: failures now include the module name and more detailed diagnostic text (when available) to help identify root causes.
    • Ensures consistent behavior when using a browser loader, preserving previous behavior for other cases.

The AMD require call in possiblyGetConfigFromAmd passed a success
callback but no error callback, causing failed module loads to be
silently swallowed. Add an explicit error callback that surfaces
the failure through the existing errorCallback mechanism.

Adversarial review: single-line change, no API surface change,
error path uses existing errorCallback pattern β€” no concerns.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

No actionable comments were generated in the recent review. πŸŽ‰

ℹ️ Recent review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4fce4d3-babd-4e1a-84b3-398b88ec15a0

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 726157b and 98ff24f.

πŸ“’ Files selected for processing (1)
  • packages/utils.component/src/loaders.ts

πŸ“ Walkthrough

Walkthrough

When config.require is a string and an AMD loader (window.amdRequire or window.require) exists, the loader is stored in amdRequire and invoked with an added error callback. On failure the error handler calls errorCallback with "Failed to load AMD module: " plus the original error message when available.

Changes

Cohort / File(s) Summary
AMD Loader Error Handling
packages/utils.component/src/loaders.ts
Store detected AMD loader in amdRequire and call it with a third argument: an error handler. On AMD load failure, invoke errorCallback with a message prefixed by Failed to load AMD module: <moduleName> and append the underlying error text when present.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

πŸ‡ I hopped through code at break of dawn,

found a module that would not respond.
I added a handler, sharp and quick,
now failures tell us what went sick.
Debugging carrotsβ€”one sweet bite!

πŸš₯ Pre-merge checks | βœ… 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
βœ… Passed checks (4 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title 'Add error callback to AMD require in component loader' (minus emoji) accurately describes the main changeβ€”adding error handling to AMD module loading.
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 fix/amd-require-error-callback

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.

Copy link
Copy Markdown
Contributor

@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: 1

πŸ€– Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/utils.component/src/loaders.ts`:
- Around line 278-280: Remove the leading semicolon before the AMD loader call
and make the err handling robust so non-Error truthy values don't produce "β€”
undefined"; specifically, change the call that uses (window.amdRequire ||
window.require)([config.require], callback, function (err) {
errorCallback('Failed to load AMD module: ' + config.require + (err ? ' β€” ' +
err.message : '')) }) to omit the initial semicolon and build the trailing error
text defensively (e.g., use err?.message ?? String(err) or similar) when
invoking errorCallback so config.require, callback,
window.amdRequire/window.require, and errorCallback are used directly and no " β€”
undefined" is emitted.
πŸͺ„ Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c386549-cd24-474a-9470-5bd14eb9c578

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f670aa8 and 726157b.

πŸ“’ Files selected for processing (1)
  • packages/utils.component/src/loaders.ts

Comment thread packages/utils.component/src/loaders.ts Outdated
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@brianmhunt brianmhunt merged commit c36af2e into main Apr 27, 2026
9 checks passed
@brianmhunt brianmhunt deleted the fix/amd-require-error-callback branch April 27, 2026 12:57
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