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: add remote-entry script resource retry for retry-plugin #3321

Merged
merged 7 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/small-eyes-change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@module-federation/retry-plugin': patch
'@module-federation/runtime': patch
---

feat: add remote-entry script resource retry for retry-plugin
2 changes: 1 addition & 1 deletion apps/router-demo/router-host-2000/rsbuild.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export default defineConfig({
shared: ['react', 'react-dom', 'antd'],
runtimePlugins: [
path.join(__dirname, './src/runtime-plugin/shared-strategy.ts'),
// path.join(__dirname, './src/runtime-plugin/retry.ts'),
path.join(__dirname, './src/runtime-plugin/retry.ts'),
],
// bridge: {
// disableAlias: true,
Expand Down
32 changes: 16 additions & 16 deletions apps/router-demo/router-host-2000/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,22 @@ init({
remotes: [],
plugins: [
BridgeReactPlugin(),
RetryPlugin({
fetch: {
url: 'http://localhost:2008/not-exist-mf-manifest.json',
fallback: () => 'http://localhost:2001/mf-manifest.json',
},
script: {
retryTimes: 3,
retryDelay: 1000,
moduleName: ['remote1'],
cb: (resolve, error) => {
return setTimeout(() => {
resolve(error);
}, 1000);
},
},
}),
// RetryPlugin({
danpeen marked this conversation as resolved.
Show resolved Hide resolved
// fetch: {
// url: 'http://localhost:2008/not-exist-mf-manifest.json',
// fallback: () => 'http://localhost:2001/mf-manifest.json',
// },
// script: {
// retryTimes: 3,
// retryDelay: 1000,
// moduleName: ['remote1'],
// cb: (resolve, error) => {
// return setTimeout(() => {
// resolve(error);
// }, 1000);
// },
// },
// }),
],
});

Expand Down
1 change: 1 addition & 0 deletions packages/retry-plugin/src/fetch-retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ async function fetchWithRetry({
logger.log(
`>>>>>>>>> retry failed after ${retryTimes} times for url: ${url}, now will try fallbackUrl url <<<<<<<<<`,
);

if (fallback && typeof fallback === 'function') {
return fetchWithRetry({
url: fallback(url),
Expand Down
66 changes: 33 additions & 33 deletions packages/retry-plugin/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { FederationRuntimePlugin } from '@module-federation/runtime/types';
import { fetchWithRetry } from './fetch-retry';
import { defaultRetries, defaultRetryDelay } from './constant';
import type { RetryPluginParams } from './types';
import { scriptCommonRetry } from './util';

const RetryPlugin: (params: RetryPluginParams) => FederationRuntimePlugin = ({
fetch: fetchOption,
Expand Down Expand Up @@ -36,39 +36,39 @@ const RetryPlugin: (params: RetryPluginParams) => FederationRuntimePlugin = ({
}
return fetch(url, options);
},
async getModuleFactory({ remoteEntryExports, expose, moduleInfo }) {
let moduleFactory;
const { retryTimes = defaultRetries, retryDelay = defaultRetryDelay } =
scriptOption || {};

if (
(scriptOption?.moduleName &&
scriptOption?.moduleName.some(
(m) => moduleInfo.name === m || (moduleInfo as any)?.alias === m,
)) ||
scriptOption?.moduleName === undefined
) {
let attempts = 0;

while (attempts - 1 < retryTimes) {
try {
moduleFactory = await remoteEntryExports.get(expose);
break;
} catch (error) {
attempts++;
if (attempts - 1 >= retryTimes) {
scriptOption?.cb &&
(await new Promise(
(resolve) =>
scriptOption?.cb && scriptOption?.cb(resolve, error),
));
throw error;
}
await new Promise((resolve) => setTimeout(resolve, retryDelay));
}
}
}
return moduleFactory;
async getRemoteEntryExports({
getRemoteEntry,
origin,
remoteInfo,
remoteEntryExports,
globalLoading,
uniqueKey,
}) {
if (!scriptOption) return;
const retryFn = getRemoteEntry;
const beforeExecuteRetry = () => delete globalLoading[uniqueKey];
const getRemoteEntryRetry = scriptCommonRetry({
scriptOption,
moduleInfo: remoteInfo,
retryFn,
beforeExecuteRetry,
});
return getRemoteEntryRetry({
origin,
remoteInfo,
remoteEntryExports,
});
},
async getModuleFactory({ remoteEntryExports, expose, moduleInfo }) {
if (!scriptOption) return;
const retryFn = remoteEntryExports.get;
const getRemoteEntryRetry = scriptCommonRetry({
scriptOption,
moduleInfo,
retryFn,
});
return getRemoteEntryRetry(expose);
},
});

Expand Down
8 changes: 8 additions & 0 deletions packages/retry-plugin/src/types.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { RemoteInfo } from '@module-federation/runtime/types';
export interface FetchWithRetryOptions {
url?: string;
options?: RequestInit;
Expand All @@ -24,3 +25,10 @@ export type RequiredFetchWithRetryOptions = Required<
Pick<FetchWithRetryOptions, 'url'>
> &
Omit<FetchWithRetryOptions, 'url'>;

export type ScriptCommonRetryOption = {
scriptOption: ScriptWithRetryOptions;
moduleInfo: RemoteInfo & { alias?: string };
retryFn: (...args: any[]) => Promise<any> | (() => Promise<any>);
beforeExecuteRetry?: (...args: any[]) => void;
};
45 changes: 45 additions & 0 deletions packages/retry-plugin/src/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { defaultRetries, defaultRetryDelay } from './constant';
import type { ScriptCommonRetryOption } from './types';
import logger from './logger';

export function scriptCommonRetry<T extends (...args: any[]) => void>({
scriptOption,
moduleInfo,
retryFn,
beforeExecuteRetry = () => {},
}: ScriptCommonRetryOption) {
return async function (...args: Parameters<T>) {
let retryResponse;
const { retryTimes = defaultRetries, retryDelay = defaultRetryDelay } =
scriptOption || {};
if (
(scriptOption?.moduleName &&
scriptOption?.moduleName.some(
(m) => moduleInfo.name === m || moduleInfo?.alias === m,
)) ||
scriptOption?.moduleName === undefined
) {
let attempts = 0;
while (attempts - 1 < retryTimes) {
try {
beforeExecuteRetry && beforeExecuteRetry();
retryResponse = await retryFn(...args);
break;
} catch (error) {
attempts++;
if (attempts - 1 >= retryTimes) {
scriptOption?.cb &&
(await new Promise(
(resolve) =>
scriptOption?.cb && scriptOption?.cb(resolve, error),
));
throw error;
}
logger.log(`script resource retrying ${attempts} times`);
await new Promise((resolve) => setTimeout(resolve, retryDelay));
}
}
}
return retryResponse;
};
}
18 changes: 17 additions & 1 deletion packages/runtime/src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
InitTokens,
CallFrom,
} from './type';
import { getBuilderId, registerPlugins } from './utils';
import { getBuilderId, registerPlugins, getRemoteEntry } from './utils';
import { Module } from './module';
import {
AsyncHook,
Expand Down Expand Up @@ -115,6 +115,22 @@ export class FederationHost {
[string, RequestInit],
Promise<Response> | void | false
>(),
Comment on lines 115 to 117
Copy link
Contributor

Choose a reason for hiding this comment

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

The AsyncHook type parameters could be more descriptive. Consider using a type alias to better document the purpose of this hook:

Suggested change
[string, RequestInit],
Promise<Response> | void | false
>(),
type RemoteEntryRequestHookParams = [string, RequestInit];
type RemoteEntryRequestResult = Promise<Response> | void | false;
requestRemoteEntry: new AsyncHook<
RemoteEntryRequestHookParams,
RemoteEntryRequestResult
>(),

getRemoteEntryExports: new AsyncHook<
[
{
getRemoteEntry: typeof getRemoteEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

The loadEntryError hook's parameter structure appears to be incomplete and lacks type information. Consider adding proper type definitions for error handling:

Suggested change
getRemoteEntryExports: new AsyncHook<
[
{
getRemoteEntry: typeof getRemoteEntry;
loadEntryError: new AsyncHook<
[{
error: Error;
origin: string;
retryCount?: number;
}],
void
>(),

origin: FederationHost;
remoteInfo: RemoteInfo;
remoteEntryExports?: RemoteEntryExports | undefined;
globalLoading: Record<
string,
Promise<void | RemoteEntryExports> | undefined
>;
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
uniqueKey: string;
},
],
Promise<(() => Promise<RemoteEntryExports | undefined>) | undefined>
>(),
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
getModuleFactory: new AsyncHook<
[
{
Expand Down
32 changes: 24 additions & 8 deletions packages/runtime/src/module/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import {
RUNTIME_002,
runtimeDescMap,
} from '@module-federation/error-codes';
import { getRemoteEntry } from '../utils/load';
import { getRemoteEntry, getRemoteEntryUniqueKey } from '../utils/load';
import { FederationHost } from '../core';
import { RemoteEntryExports, RemoteInfo, InitScope } from '../type';
import { globalLoading } from '../global';

export type ModuleOptions = ConstructorParameters<typeof Module>[0];
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -34,18 +35,33 @@ class Module {
return this.remoteEntryExports;
}

// Get remoteEntry.js
const remoteEntryExports = await getRemoteEntry({
origin: this.host,
remoteInfo: this.remoteInfo,
remoteEntryExports: this.remoteEntryExports,
});
let remoteEntryExports;
Copy link
Contributor

Choose a reason for hiding this comment

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

The remoteEntryExports variable declaration could be more strictly typed to match its expected type. Add explicit typing:

Suggested change
let remoteEntryExports;
let remoteEntryExports: RemoteEntryExports | undefined;

const uniqueKey = getRemoteEntryUniqueKey(this.remoteInfo);
remoteEntryExports =
await this.host.loaderHook.lifecycle.getRemoteEntryExports.emit({
Copy link
Member

Choose a reason for hiding this comment

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

it looks like the same as loadEntry hook

getRemoteEntry,
origin: this.host,
remoteInfo: this.remoteInfo,
Copy link
Contributor

Choose a reason for hiding this comment

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

The initial getRemoteEntry call could benefit from more specific error typing and context. Consider adding a custom error type:

Suggested change
const uniqueKey = getRemoteEntryUniqueKey(this.remoteInfo);
remoteEntryExports =
await this.host.loaderHook.lifecycle.getRemoteEntryExports.emit({
getRemoteEntry,
origin: this.host,
remoteInfo: this.remoteInfo,
try {
remoteEntryExports = await getRemoteEntry({
origin: this.host,
remoteInfo: this.remoteInfo,
remoteEntryExports: this.remoteEntryExports,
}) as RemoteEntryExports;
} catch (err: unknown) {

remoteEntryExports: this.remoteEntryExports,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add logging for debugging and monitoring purposes before entering error recovery:

Suggested change
remoteEntryExports: this.remoteEntryExports,
} catch (err) {
console.warn(`Remote entry load failed for ${this.remoteInfo.name}, attempting recovery:`, err);

globalLoading,
uniqueKey,
});
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved

// get exposeGetter
if (!remoteEntryExports) {
remoteEntryExports = await getRemoteEntry({
origin: this.host,
remoteInfo: this.remoteInfo,
remoteEntryExports: this.remoteEntryExports,
});
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 recovery flow could benefit from a timeout to prevent hanging in case the error handler itself fails. Add a timeout wrapper:

Suggested change
uniqueKey,
});
// get exposeGetter
if (!remoteEntryExports) {
remoteEntryExports = await getRemoteEntry({
origin: this.host,
remoteInfo: this.remoteInfo,
remoteEntryExports: this.remoteEntryExports,
});
const RECOVERY_TIMEOUT = 15000; // 15 seconds timeout
remoteEntryExports = await Promise.race([
this.host.loaderHook.lifecycle.loadEntryError.emit({
getRemoteEntry,
origin: this.host,
remoteInfo: this.remoteInfo,
remoteEntryExports: this.remoteEntryExports,
globalLoading,
uniqueKey,
}),
new Promise((_, reject) =>
setTimeout(() => reject(new Error('Error recovery timeout')), RECOVERY_TIMEOUT)
)
]);

}
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved

assert(
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
remoteEntryExports,
`remoteEntryExports is undefined \n ${safeToString(this.remoteInfo)}`,
);
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved

this.remoteEntryExports = remoteEntryExports;
this.remoteEntryExports = remoteEntryExports as RemoteEntryExports;
zhoushaw marked this conversation as resolved.
Show resolved Hide resolved
return this.remoteEntryExports;
}

Expand Down
Loading