fix(runtime): throw RUNTIME-008 Error when script resources load failed #3348
Conversation
🦋 Changeset detectedLatest commit: e1c3af4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![squadronai[bot]](https://avatars.githubusercontent.com/in/901096?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This pull request introduces a fix to improve error handling when script resources fail to load. The key changes are:
- Throw a specific
RUNTIME-008error when script resources fail to load, providing users with more informative error messages. - In the SDK, the
createScriptfunction now accepts anonErrorCallbackoption, which is called when a script fails to load. Additionally, the timeout duration for script loading has been increased from 20 seconds to 60 seconds. - These changes aim to enhance the overall user experience by giving them more detailed information when script resource loading issues occur, helping them better identify and resolve such problems.
File Summaries
| File | Summary |
|---|---|
| packages/runtime/src/utils/load.ts | The code changes introduce a new error handling mechanism to throw a specific RUNTIME-008 error when script resources fail to load. This ensures that users receive more informative error messages when encountering resource loading issues. |
| packages/sdk/src/dom.ts | The code changes introduce an error handling mechanism for script resource loading failures. The createScript function now accepts an onErrorCallback option, which is called when a script fails to load. Additionally, the timeout duration for script loading has been increased from 20 seconds to 60 seconds. These changes aim to provide users with more detailed error information when script resources fail to load. |
danpeen
changed the title
There was a problem hiding this comment.
Incremental Review
Comments posted: 6
Configuration
Squadron Mode: essential
Commits Reviewed
3ae535306492443fc17066a1d778dad9ad5939eb...4d840084169876e812336cd983c0047724c9a854
Files Reviewed
- packages/runtime/src/utils/load.ts
- packages/sdk/src/dom.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/smart-crabs-burn.md
- packages/error-codes/src/desc.ts
- packages/error-codes/src/error-codes.ts
packages/runtime/src/utils/load.ts
Outdated
| .catch((e) => { | ||
| // const errMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | ||
| // remoteName: name, | ||
| // remoteEntryUrl: entry, | ||
| // }); | ||
| assert( | ||
| undefined, | ||
| getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | ||
| remoteName: name, | ||
| resourceUrl: entry, |
There was a problem hiding this comment.
The error handling here can be improved. Currently, you're asserting undefined which will always trigger the error, then throwing the original error. This creates two error messages and makes debugging harder. Instead, create a custom error with the formatted message:
| .catch((e) => { | |
| // const errMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | |
| // remoteName: name, | |
| // remoteEntryUrl: entry, | |
| // }); | |
| assert( | |
| undefined, | |
| getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | |
| remoteName: name, | |
| resourceUrl: entry, | |
| .catch((e) => { | |
| const errorMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | |
| remoteName: name, | |
| resourceUrl: entry, | |
| }); | |
| throw new Error(errorMsg); | |
| }); |
This approach:
- Creates a single, clear error message
- Preserves the error context
- Is more efficient than asserting and throwing separately
packages/runtime/src/utils/load.ts
Outdated
| .catch((e) => { | ||
| // const errMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | ||
| // remoteName: name, | ||
| // remoteEntryUrl: entry, | ||
| // }); | ||
| assert( | ||
| undefined, | ||
| getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | ||
| remoteName: name, | ||
| resourceUrl: entry, |
There was a problem hiding this comment.
Consider adding error type checking to provide more specific error messages for different failure scenarios. For example, network errors might need different handling than syntax errors in the loaded script:
| .catch((e) => { | |
| // const errMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | |
| // remoteName: name, | |
| // remoteEntryUrl: entry, | |
| // }); | |
| assert( | |
| undefined, | |
| getShortErrorMsg(RUNTIME_008, runtimeDescMap, { | |
| remoteName: name, | |
| resourceUrl: entry, | |
| .catch((e) => { | |
| const details = { | |
| remoteName: name, | |
| resourceUrl: entry, | |
| errorType: e instanceof Error ? e.name : 'Unknown', | |
| }; | |
| const errorMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, details); | |
| throw new Error(errorMsg); | |
| }); |
packages/sdk/src/dom.ts
Outdated
| clearTimeout(timeoutId); | ||
| const onScriptCompleteCallback = () => { | ||
| if (event.type === 'error') { | ||
| info?.cb?.reject && info?.cb?.reject(event); | ||
| } else { |
There was a problem hiding this comment.
The error handling logic should throw a specific error code (RUNTIME-008) as mentioned in the PR title. Currently, it's just passing the error to the callback. Modify the error handling to:
| clearTimeout(timeoutId); | |
| const onScriptCompleteCallback = () => { | |
| if (event.type === 'error') { | |
| info?.cb?.reject && info?.cb?.reject(event); | |
| } else { | |
| if (event.type === 'error') { | |
| const error = new Error(`Failed to load script: ${info.url}`); | |
| error.code = 'RUNTIME-008'; | |
| info?.onErrorCallback && info?.onErrorCallback(error); | |
| } else { | |
| info?.cb && info?.cb(); | |
| } |
packages/sdk/src/dom.ts
Outdated
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| event: any, | ||
| ): void => { | ||
| const onLinkCompleteCallback = () => { | ||
| if (event.type === 'error') { |
There was a problem hiding this comment.
For consistency with the script error handling, the link error handling should also use the RUNTIME-008 error code:
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| event: any, | |
| ): void => { | |
| const onLinkCompleteCallback = () => { | |
| if (event.type === 'error') { | |
| if (event.type === 'error') { | |
| const error = new Error(`Failed to load link: ${info.url}`); | |
| error.code = 'RUNTIME-008'; | |
| info?.onErrorCallback && info?.onErrorCallback(error); | |
| } else { | |
| info?.cb && info?.cb(); | |
| } |
packages/sdk/src/dom.ts
Outdated
| cb?: { | ||
| resolve: (value: void | PromiseLike<void>) => void; | ||
| reject: (error: Error) => void; | ||
| }; | ||
| attrs?: Record<string, any>; | ||
| needDeleteScript?: boolean; | ||
| createScriptHook?: CreateScriptHookDom; | ||
| }): { script: HTMLScriptElement; needAttach: boolean } { | ||
| // Retrieve the existing script element by its src attribute | ||
| let script: HTMLScriptElement | null = null; |
There was a problem hiding this comment.
The timeout value should be configurable through the info parameter rather than hardcoded. Consider adding a timeout option to the info parameter interface.
There was a problem hiding this comment.
Incremental Review
Comments posted: 5
Configuration
Squadron Mode: essential
Commits Reviewed
3ae535306492443fc17066a1d778dad9ad5939eb...df9adea20c412b2b828d64aa7c4a4fd7142ec6b0
Files Reviewed
- packages/runtime/src/utils/load.ts
- packages/sdk/src/dom.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/smart-crabs-burn.md
- packages/error-codes/src/desc.ts
- packages/error-codes/src/error-codes.ts
packages/sdk/src/dom.ts
Outdated
| attrs?: Record<string, any>; | ||
| needDeleteScript?: boolean; | ||
| createScriptHook?: CreateScriptHookDom; | ||
| }): { script: HTMLScriptElement; needAttach: boolean } { | ||
| // Retrieve the existing script element by its src attribute | ||
| let script: HTMLScriptElement | null = null; | ||
| let needAttach = true; | ||
| let timeout = 20000; | ||
| let timeout = 60000; |
There was a problem hiding this comment.
The timeout value should be declared as a const since it's not being reassigned within the scope. Consider: ```suggestion
const timeout = 60000;
packages/sdk/src/dom.ts
Outdated
| if (event.type === 'error') { | ||
| info?.onErrorCallback && info?.onErrorCallback(event); | ||
| } else { | ||
| info?.cb && info?.cb(); | ||
| } |
There was a problem hiding this comment.
The optional chaining operator (?.) is used redundantly with the && operator. Simplify to: ```suggestion
if (event.type === 'error') {
info.onErrorCallback?.(event);
} else {
info.cb?.();
}
packages/sdk/src/dom.ts
Outdated
| if (event.type === 'error') { | ||
| info?.onErrorCallback && info?.onErrorCallback(event); | ||
| } else { | ||
| info?.cb && info?.cb(); | ||
| } |
There was a problem hiding this comment.
Similar to the script handler, simplify the optional chaining: ```suggestion
if (event.type === 'error') {
info.onErrorCallback?.(event);
} else {
info.cb?.();
}
| attrs: { | ||
| fetchpriority: 'high', | ||
| ...attrs, |
There was a problem hiding this comment.
The fetchpriority attribute should be included in the TypeScript interface definition for attrs to ensure type safety. Consider documenting why high priority is needed as a default.
|
just my 2 cents, I would suggest to make RUNTIME errors more memorable and move from the In this case, e.g. wdyt? |
Description
fix(runtime): In order for users to receive resource loading errors, throw RUNTIME-008 Error when script resources load failed
Related Issue
Types of changes
Checklist