fix(retry-plugin): error-boundary should render until script retry finished#3127
fix(retry-plugin): error-boundary should render until script retry finished#3127
Conversation
🦋 Changeset detectedLatest commit: d49710d The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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. |
There was a problem hiding this comment.
Summary
This pull request introduces a fix to the error-boundary component in the retry-plugin, ensuring that it continues to render until the script retry process is finished. The changes aim to improve the user experience by preventing the component from prematurely unmounting during the retry operation.
The key changes include:
- Introducing a new
getModuleFactoryhook in theFederationHostclass, which is responsible for fetching the module factory for a remote entry. This is necessary for handling script retries in the error boundary. - Fixing the issue where the error-boundary component would unmount before the script retry was completed, leading to an inconsistent user experience.
- Ensuring that the error-boundary component continues to render until the script retry process is finished, providing a more seamless and reliable application behavior.
These changes are focused on improving the integration of the retry-plugin with the existing codebase, enhancing the overall reliability and user experience of the application.
File Summaries
| File | Summary |
|---|---|
| packages/runtime/src/core.ts | The code changes introduce a new getModuleFactory hook in the FederationHost class. This hook is responsible for fetching the module factory for a remote entry, which is necessary for handling script retries in the error boundary. The changes aim to ensure that the error boundary continues to render until the script retry is finished, providing a better user experience. |
| packages/runtime/src/module/index.ts | The code changes introduce a fix to ensure that the error-boundary component continues to render until the script retry process is finished. This change ensures that the application's behavior aligns with the expected user experience, as depicted in the provided image. |
| packages/runtime/src/remote/index.ts | The code changes introduce a fix to ensure that the error-boundary component continues to render until the script retry process is finished. This ensures a consistent user experience by preventing the component from prematurely unmounting during the retry operation. |
There was a problem hiding this comment.
Incremental Review
Comments posted: 7
Configuration
Squadron Mode: essential
Commits Reviewed
87a28620bc9139b07365b6755addb1924333e930...2d1c9fa08c658bcbd3e916bea49663d3edb7c745
Files Reviewed
- packages/runtime/src/remote/index.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/core.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/blue-poets-play.md
- apps/router-demo/router-host-2000/src/App.tsx
- apps/router-demo/router-host-2000/src/runtime-plugin/retry.ts
- apps/website-new/docs/en/plugin/plugins/retry-plugin.mdx
- apps/website-new/docs/zh/plugin/plugins/retry-plugin.mdx
- packages/retry-plugin/src/constant.ts
- packages/retry-plugin/src/fetch-retry.ts
- packages/retry-plugin/src/index.ts
- packages/retry-plugin/src/types.d.ts
| const moduleOrFactory = (await module.get( | ||
| idRes, | ||
| expose, |
There was a problem hiding this comment.
The module.get() call should include the optional parameters to ensure proper error handling during retries. Add the loadFactory and remoteSnapshot parameters explicitly:
| const moduleOrFactory = (await module.get( | |
| idRes, | |
| expose, | |
| const moduleOrFactory = (await module.get( | |
| idRes, | |
| expose, | |
| { loadFactory: true }, | |
| remoteSnapshot | |
| ))) |
This ensures consistent behavior with the retry plugin and error boundary, making the parameters explicit rather than relying on defaults.
| const moduleOrFactory = (await module.get( | ||
| idRes, | ||
| expose, |
There was a problem hiding this comment.
Consider wrapping the module.get() call in a try-catch block to handle potential failures during the remote module loading, especially since this is related to the retry plugin:
| const moduleOrFactory = (await module.get( | |
| idRes, | |
| expose, | |
| try { | |
| const moduleOrFactory = (await module.get( | |
| idRes, | |
| expose, | |
| { loadFactory: true }, | |
| remoteSnapshot | |
| )); | |
| } catch (error) { | |
| // Let the error boundary handle the retry | |
| throw error; | |
| } |
This makes error handling more explicit and ensures proper integration with the error boundary's retry mechanism.
| let moduleFactory; | ||
| moduleFactory = await this.host.loaderHook.lifecycle.getModuleFactory.emit({ | ||
| remoteEntryExports, | ||
| expose, | ||
| moduleInfo: this.remoteInfo, | ||
| }); |
There was a problem hiding this comment.
The moduleFactory assignment could benefit from destructuring and more descriptive variable naming. Here's a clearer way to write it:
| let moduleFactory; | |
| moduleFactory = await this.host.loaderHook.lifecycle.getModuleFactory.emit({ | |
| remoteEntryExports, | |
| expose, | |
| moduleInfo: this.remoteInfo, | |
| }); | |
| const { moduleFactory: hookModuleFactory } = await this.host.loaderHook.lifecycle.getModuleFactory.emit({ | |
| remoteEntryExports, | |
| expose, | |
| moduleInfo: this.remoteInfo, | |
| }); | |
| let moduleFactory = hookModuleFactory; |
This makes it clearer that we're getting the moduleFactory from the hook and allows for better tracking of where the value comes from.
| if (!moduleFactory) { | ||
| moduleFactory = await remoteEntryExports.get(expose); | ||
| } |
There was a problem hiding this comment.
The fallback logic for moduleFactory could be more robust with error handling. Consider adding a try-catch block:
| if (!moduleFactory) { | |
| moduleFactory = await remoteEntryExports.get(expose); | |
| } | |
| if (!moduleFactory) { | |
| try { | |
| moduleFactory = await remoteEntryExports.get(expose); | |
| } catch (error) { | |
| throw new Error(`Failed to get module factory for ${expose}: ${error.message}`); | |
| } | |
| } |
| assert( | ||
| moduleFactory, | ||
| `${getFMId(this.remoteInfo)} remote don't export ${expose}.`, |
There was a problem hiding this comment.
The assertion message appears to be truncated. Ensure the error message provides complete information about what went wrong:
| assert( | |
| moduleFactory, | |
| `${getFMId(this.remoteInfo)} remote don't export ${expose}.`, | |
| assert( | |
| moduleFactory, | |
| `${getFMId(this.remoteInfo)} remote module factory not found for expose "${expose}"` | |
| ); |
| getModuleFactory: new AsyncHook< | ||
| [ | ||
| { | ||
| remoteEntryExports: RemoteEntryExports; | ||
| expose: string; | ||
| moduleInfo: RemoteInfo; | ||
| }, | ||
| ], | ||
| Promise<(() => Promise<Module>) | undefined> | ||
| >(), |
There was a problem hiding this comment.
The AsyncHook type definition could be improved for better type safety and readability. Consider adding more specific types for the module factory return value:
| getModuleFactory: new AsyncHook< | |
| [ | |
| { | |
| remoteEntryExports: RemoteEntryExports; | |
| expose: string; | |
| moduleInfo: RemoteInfo; | |
| }, | |
| ], | |
| Promise<(() => Promise<Module>) | undefined> | |
| >(), | |
| getModuleFactory: new AsyncHook< | |
| [{ | |
| remoteEntryExports: RemoteEntryExports; | |
| expose: string; | |
| moduleInfo: RemoteInfo; | |
| }], | |
| Promise<ModuleFactory | undefined> | |
| >; | |
| type ModuleFactory = () => Promise<Module>; |
This makes the return type more explicit and self-documenting, while also allowing for better type checking and IDE support.
| [string, RequestInit], | ||
| Promise<Response> | void | false | ||
| >(), |
There was a problem hiding this comment.
The first type parameter appears to be for a request interceptor. Consider adding a descriptive type alias to improve code readability:
| [string, RequestInit], | |
| Promise<Response> | void | false | |
| >(), | |
| type RequestInterceptor = [string, RequestInit]; | |
| // Then use it as: | |
| RequestInterceptor, | |
| Promise<Response> | void | false |
This makes the purpose of the tuple type more clear and maintainable.
Description
fix: error-boundary should render until script retry finished. after fixing it will act like this:
Related Issue
Types of changes
Checklist