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.

Sign up for GitHub

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

fix(runtime): throw RUNTIME-008 Error when script resources load failed #3348

Merged
merged 8 commits into from
Dec 27, 2024
Merged

fix(runtime): throw RUNTIME-008 Error when script resources load failed #3348

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d03b49c
fix: throw RUNTIME-008 Error when script resources load failed
danpeen Dec 12, 2024
4d84008
chore: merge main
danpeen Dec 12, 2024
4d89133
chore: delete useless code
danpeen Dec 12, 2024
df9adea
fix: fix ci build error
danpeen Dec 12, 2024
ef040e3
chore: merge main
danpeen Dec 27, 2024
1357b04
fix: reset timeout setting
danpeen Dec 27, 2024
2ac14a3
chore: fix sdk tests failed
danpeen Dec 27, 2024
e1c3af4
chore: fix runtime tests failed
danpeen Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/smart-crabs-burn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@module-federation/error-codes': patch
'@module-federation/runtime': patch
'@module-federation/sdk': patch
---

fix: throw RUNTIME-008 Error when script resources load failed
2 changes: 2 additions & 0 deletions packages/error-codes/src/desc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
RUNTIME_005,
RUNTIME_006,
RUNTIME_007,
RUNTIME_008,
TYPE_001,
} from './error-codes';

Expand All @@ -17,6 +18,7 @@ export const runtimeDescMap = {
[RUNTIME_005]: 'Invalid loadShareSync function call from bundler runtime',
[RUNTIME_006]: 'Invalid loadShareSync function call from runtime',
[RUNTIME_007]: 'Failed to get remote snapshot.',
[RUNTIME_008]: 'Failed to load script resources.',
};

export const typeDescMap = {
Expand Down
1 change: 1 addition & 0 deletions packages/error-codes/src/error-codes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ export const RUNTIME_004 = 'RUNTIME-004';
export const RUNTIME_005 = 'RUNTIME-005';
export const RUNTIME_006 = 'RUNTIME-006';
export const RUNTIME_007 = 'RUNTIME-007';
export const RUNTIME_008 = 'RUNTIME-008';

export const TYPE_001 = 'TYPE-001';
14 changes: 14 additions & 0 deletions packages/runtime/src/utils/load.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Remote, RemoteEntryExports, RemoteInfo } from '../type';
import { assert } from './logger';
import {
RUNTIME_001,
RUNTIME_008,
getShortErrorMsg,
runtimeDescMap,
} from '@module-federation/error-codes';
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -129,6 +130,19 @@ async function loadEntryScript({
return entryExports;
})
.catch((e) => {
// const errMsg = getShortErrorMsg(RUNTIME_008, runtimeDescMap, {
// remoteName: name,
// remoteEntryUrl: entry,
// });
assert(
undefined,
getShortErrorMsg(RUNTIME_008, runtimeDescMap, {
remoteName: name,
resourceUrl: entry,
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
.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:

  1. Creates a single, clear error message
  2. Preserves the error context
  3. Is more efficient than asserting and throwing separately

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
.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);
});

}),
);
// console.error('----loadScript error-----', entry);
// console.error(errMsg);
throw e;
});
}
Expand Down
45 changes: 35 additions & 10 deletions packages/sdk/src/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,21 @@ export function isStaticResourcesEqual(url1: string, url2: string): boolean {

export function createScript(info: {
url: string;
cb?: (value: void | PromiseLike<void>) => void;
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

The timeout value should be configurable through the info parameter rather than hardcoded. Consider adding a timeout option to the info parameter interface.

let needAttach = true;
let timeout = 20000;
let timeout = 60000;
Copy link
Contributor

Choose a reason for hiding this comment

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

The timeout value should be declared as a const since it's not being reassigned within the scope. Consider: ```suggestion
const timeout = 60000;

let timeoutId: NodeJS.Timeout;
const scripts = document.getElementsByTagName('script');

for (let i = 0; i < scripts.length; i++) {
const s = scripts[i];
const scriptSrc = s.getAttribute('src');
Expand Down Expand Up @@ -88,6 +92,14 @@ export function createScript(info: {
event: any,
): Promise<void> => {
clearTimeout(timeoutId);
const onScriptCompleteCallback = () => {
if (event.type === 'error') {
info?.cb?.reject && info?.cb?.reject(event);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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();
}

info?.cb?.resolve && info?.cb?.resolve();
}
};

// Prevent memory leaks in IE.
if (script) {
script.onerror = null;
Expand All @@ -102,14 +114,14 @@ export function createScript(info: {
const result = (prev as any)(event);
if (result instanceof Promise) {
const res = await result;
info?.cb?.();
onScriptCompleteCallback();
return res;
}
info?.cb?.();
onScriptCompleteCallback();
return result;
}
}
info?.cb?.();
onScriptCompleteCallback();
};

script.onerror = onScriptComplete.bind(null, script.onerror);
Expand All @@ -127,7 +139,10 @@ export function createScript(info: {

export function createLink(info: {
url: string;
cb: (value: void | PromiseLike<void>) => void;
cb?: {
resolve: (value: void | PromiseLike<void>) => void;
reject: (error: Error) => void;
};
attrs: Record<string, string>;
needDeleteLink?: boolean;
createLinkHook?: (
Expand Down Expand Up @@ -184,6 +199,13 @@ export function createLink(info: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
event: any,
): void => {
const onLinkCompleteCallback = () => {
if (event.type === 'error') {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the script error handling, the link error handling should also use the RUNTIME-008 error code:

Suggested change
// 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();
}

info?.cb?.reject && info?.cb?.reject(event);
} else {
info?.cb?.resolve && info?.cb?.resolve();
}
};
// Prevent memory leaks in IE.
if (link) {
link.onerror = null;
Expand All @@ -197,11 +219,11 @@ export function createLink(info: {
if (prev) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const res = (prev as any)(event);
info.cb();
onLinkCompleteCallback();
return res;
}
}
info.cb();
onLinkCompleteCallback();
};

link.onerror = onLinkComplete.bind(null, link.onerror);
Expand All @@ -218,10 +240,13 @@ export function loadScript(
},
) {
const { attrs = {}, createScriptHook } = info;
return new Promise<void>((resolve, _reject) => {
return new Promise<void>((resolve, reject) => {
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
const { script, needAttach } = createScript({
url,
cb: resolve,
cb: {
resolve,
reject,
},
attrs: {
fetchpriority: 'high',
...attrs,
Comment on lines 244 to 246
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Expand Down
Loading