feat: add bridgeHook plugin system to support lifecycyle in bridge #2992
Conversation
…universe into feat/isolated-monitor
🦋 Changeset detectedLatest commit: b25a91b 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
The pull request introduces a new plugin system called "bridgeHook" to the module-federation bridge, allowing for the addition of lifecycle hooks to support more use cases. The changes include the following key points:
- Addition of four new hooks:
beforeBridgeRender,afterBridgeRender,beforeBridgeDestroy, andafterBridgeDestroy, which can be used to add custom logic before and after the rendering and destruction of the bridge component. - Ability to pass additional props to the bridge component through the
beforeBridgeRenderhook. - Integration of the
ProviderParamstype from thebridge-sharedlibrary to support the new plugin system. - Enhancements to the
RemoteAppWrappercomponent to support the newbridgeHookplugin system. - Updates to the
createBrowserRouterfunction to include a newbasenameparameter, which defaults to therouterContextProps.basenameor therouter?.basenamevalue. - Addition of the "useRoute" function from the "vue-router" package and updates to the "RemoteApp" component to accommodate the new plugin system in the Vue3 bridge.
- Introduction of a new
afterLoadSnapshothook in theSnapshotHandlerclass, allowing for additional processing or handling of the loaded snapshot data. - Refactoring of the existing logic for loading remote snapshots, simplifying the return values and making the code more readable.
These changes aim to provide more flexibility and extensibility to the module-federation bridge, enabling developers to handle more complex scenarios and support a wider range of use cases.
File Summaries
| File | Summary |
|---|---|
| packages/bridge/bridge-react/src/create.tsx | The code changes introduce a new plugin system for the bridge-react library, allowing for the addition of lifecycle hooks to support more use cases. The changes include the addition of an ErrorBoundary component and the integration of the ProviderParams type from the bridge-shared library. |
| packages/bridge/bridge-react/src/provider.tsx | The code changes introduce a new plugin system called "bridgeHook" to the module-federation bridge, allowing for the addition of lifecycle hooks to support more use cases. The changes include the addition of four new hooks: beforeBridgeRender, afterBridgeRender, beforeBridgeDestroy, and afterBridgeDestroy, which can be used to add custom logic before and after the rendering and destruction of the bridge component. The changes also include the ability to pass additional props to the bridge component through the beforeBridgeRender hook. |
| packages/bridge/bridge-react/src/remote/index.tsx | The code changes introduce a new plugin system for the bridge-react module, allowing for the addition of lifecycle hooks to support more use cases. The changes include the addition of two new functions, getModuleName and getRootDomDefaultClassName, which are used to generate a unique class name for the root component of the remote application. Additionally, the RemoteAppWrapper component has been updated to support the new bridgeHook plugin system, allowing for the execution of custom logic before and after the rendering and destruction of the remote component. |
| packages/bridge/bridge-react/src/router-v5.tsx | The code changes introduce a new plugin system for the bridge-react library, allowing for the addition of lifecycle hooks to support more use cases within the module-deferation bridge. The changes focus on integrating the new bridgeHook plugin system into the existing router-v5 module, enabling developers to leverage lifecycle hooks and enhance the functionality of the bridge. |
| packages/bridge/bridge-react/src/router.tsx | The changes in this file introduce a new feature to support lifecycle hooks in the module-deferation bridge, allowing for more scene support with bridge capability. The key modification is the addition of a new parameter, basename, to the createBrowserRouter function, which now defaults to the routerContextProps.basename or the router?.basename value. |
| packages/bridge/vue3-bridge/src/create.ts | The code changes introduce a new plugin system called "bridgeHook" to support lifecycle hooks in the module-deferation bridge, allowing for more flexible scene support with bridge capabilities. The changes include the addition of the "useRoute" function from the "vue-router" package and updates to the "RemoteApp" component to accommodate the new plugin system. |
| packages/bridge/vue3-bridge/src/provider.ts | The code changes introduce a new plugin system called "bridgeHook" to the module-deferation bridge, which allows for the support of lifecycle hooks in the bridge. This enables the bridge to handle more complex scenarios and provide additional functionality. |
| packages/bridge/vue3-bridge/src/remoteApp.tsx | The code changes introduce a new plugin system called "bridgeHook" to support lifecycle events in the module-federation bridge. The changes include adding hooks for "beforeBridgeRender" and "afterBridgeDestroy" events, which allow external modules to hook into the lifecycle of the remote component being rendered. These hooks are used to emit events that can be listened to by other parts of the application, enabling more flexibility and extensibility in the bridge system. |
| packages/runtime/src/core.ts | The code changes introduce a new bridgeHook plugin system to the FederationHost class, which allows for the addition of lifecycle hooks to support more scenarios with the bridge capability. The new plugin system includes four hooks: beforeBridgeRender, afterBridgeRender, beforeBridgeDestroy, and afterBridgeDestroy, which can be used to extend the functionality of the bridge. |
| packages/runtime/src/embedded.ts | The changes introduce a new bridgeHook property to the FederationHost class, which allows for the support of lifecycle hooks in the module-deferation bridge. This enhancement enables the handling of more diverse scenarios where bridge capabilities are required. |
| packages/runtime/src/module/index.ts | The code changes introduce a new feature to support lifecycle hooks in the module-deferation bridge, allowing for better support of different scenes with bridge capability. The key modifications include adding a new utility function processModuleAlias to ensure consistent module name formatting, and updating the wrapModuleFactory method to use the processed module name instead of the raw remote information. |
| packages/runtime/src/plugins/snapshot/SnapshotHandler.ts | The code changes introduce a new afterLoadSnapshot hook in the SnapshotHandler class, which is called after the remote snapshot is loaded. This allows for additional processing or handling of the loaded snapshot data. The changes also refactor the existing logic for loading remote snapshots, simplifying the return values and making the code more readable. |
| packages/runtime/src/type/config.ts | The code changes introduce a new RemoteInfoCommon interface to the config.ts file, which defines the common properties for remote information, such as alias, shareScope, and type. This change aims to support the lifecycle hooks in the module-deferation bridge, allowing for more scene-specific capabilities within the bridge. |
| packages/runtime/src/type/plugin.ts | The code changes introduce a new plugin system for the module-deferation bridge, allowing for the support of lifecycle hooks. This enables the bridge to handle more complex scenarios by providing a way to extend its functionality through custom plugins. |
| packages/runtime/src/utils/plugin.ts | The code changes introduce a new bridgeHook property to the Module['host'] interface, allowing for the registration of lifecycle hooks in the module-deferation bridge. This enhancement enables better support for various scenes that utilize the bridge capability, providing more flexibility and extensibility to the system. |
| packages/runtime/src/utils/tool.ts | The code changes introduce a new utility function called processModuleAlias that takes a module name and a sub-path, and returns a processed module name by handling various edge cases such as removing trailing slashes and handling relative sub-paths. This function is likely intended to be used in the context of module federation or a similar system where module names and paths need to be normalized. |
There was a problem hiding this comment.
Incremental Review
Comments posted: 23
Configuration
Squadron Mode: essential
Commits Reviewed
44ac346ef4212a17e4b2d611ba0e7948ba804947...4e319bb2d0708da2b1f89bad9f1b2aec4c1029c2
Files Reviewed
- packages/bridge/bridge-react/src/create.tsx
- packages/bridge/bridge-react/src/lifecycle.ts
- packages/bridge/bridge-react/src/provider.tsx
- packages/bridge/bridge-react/src/remote/index.tsx
- packages/bridge/bridge-react/src/router-v6.tsx
- packages/bridge/bridge-react/src/router.tsx
- packages/bridge/vue3-bridge/src/create.ts
- packages/bridge/vue3-bridge/src/lifecycle.ts
- packages/bridge/vue3-bridge/src/provider.ts
- packages/bridge/vue3-bridge/src/remoteApp.tsx
- packages/runtime/src/helpers.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/plugins/snapshot/SnapshotHandler.ts
- packages/runtime/src/type/config.ts
- packages/runtime/src/type/plugin.ts
- packages/runtime/src/utils/plugin.ts
- packages/runtime/src/utils/tool.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
There was a problem hiding this comment.
Incremental Review
Comments posted: 31
Configuration
Squadron Mode: essential
Commits Reviewed
8712967dd8594e99bb012c3ef5520d2e84ee3b58...cd405919b9dd79637fb73bd57e07d359739d7219
Files Reviewed
- packages/runtime/src/module/index.ts
- packages/runtime/src/plugins/snapshot/SnapshotHandler.ts
- packages/bridge/bridge-react/src/provider.tsx
- packages/bridge/vue3-bridge/src/provider.ts
- packages/bridge/bridge-react/src/create.tsx
- packages/bridge/bridge-react/src/remote/index.tsx
- packages/bridge/vue3-bridge/src/create.ts
- packages/bridge/vue3-bridge/src/remoteApp.tsx
- packages/runtime/src/utils/plugin.ts
- packages/runtime/src/utils/tool.ts
- packages/runtime/src/type/config.ts
- packages/bridge/bridge-react/src/router.tsx
- packages/runtime/src/core.ts
- packages/runtime/src/embedded.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
| // keep symbol for module name always one format | ||
| const symbolName = processModuleAlias(this.remoteInfo.name, expose); | ||
| const wrapModuleFactory = this.wraperFactory(moduleFactory, symbolName); |
There was a problem hiding this comment.
The symbolName generation and module factory wrapping could benefit from error handling to prevent potential runtime issues. Consider adding validation:
| // keep symbol for module name always one format | |
| const symbolName = processModuleAlias(this.remoteInfo.name, expose); | |
| const wrapModuleFactory = this.wraperFactory(moduleFactory, symbolName); | |
| // keep symbol for module name always one format | |
| const symbolName = processModuleAlias(this.remoteInfo.name, expose); | |
| assert(symbolName, `Failed to process module alias for ${expose}`); | |
| const wrapModuleFactory = this.wraperFactory(moduleFactory, symbolName); | |
| assert(wrapModuleFactory, `Failed to wrap module factory for ${symbolName}`); |
| if (!loadFactory) { | ||
| return wrapModuleFactory; |
There was a problem hiding this comment.
The early return logic could be more explicit about what's being returned. Add a type annotation to improve code clarity:
| if (!loadFactory) { | |
| return wrapModuleFactory; | |
| if (!loadFactory) { | |
| return wrapModuleFactory as () => Promise<any>; | |
| } |
| remoteSnapshot: ModuleInfo; | ||
| from: 'global' | 'manifest'; | ||
| }>('loadRemoteSnapshot'), | ||
| afterLoadSnapshot: new AsyncWaterfallHook<{ | ||
| options: Options; | ||
| moduleInfo: Remote; | ||
| remoteSnapshot: ModuleInfo; | ||
| }>('afterLoadSnapshot'), | ||
| }); |
There was a problem hiding this comment.
The hook definitions could benefit from more specific typing. Consider using a discriminated union type for the 'from' parameter to make the code more type-safe and self-documenting:
| remoteSnapshot: ModuleInfo; | |
| from: 'global' | 'manifest'; | |
| }>('loadRemoteSnapshot'), | |
| afterLoadSnapshot: new AsyncWaterfallHook<{ | |
| options: Options; | |
| moduleInfo: Remote; | |
| remoteSnapshot: ModuleInfo; | |
| }>('afterLoadSnapshot'), | |
| }); | |
| type SnapshotSource = { | |
| type: 'global'; | |
| } | { | |
| type: 'manifest'; | |
| manifestUrl: string; | |
| }; | |
| // Then use in hook: | |
| loadRemoteSnapshot: new AsyncWaterfallHook<{ | |
| options: Options; | |
| moduleInfo: Remote; | |
| remoteSnapshot: ModuleInfo; | |
| from: SnapshotSource; | |
| }>('loadRemoteSnapshot'), |
| let mSnapshot; | ||
| let gSnapshot; | ||
| // global snapshot includes manifest or module info includes manifest | ||
| if (globalRemoteSnapshot) { | ||
| if (isManifestProvider(globalRemoteSnapshot)) { |
There was a problem hiding this comment.
The variable names 'mSnapshot' and 'gSnapshot' are not descriptive enough. Consider using more meaningful names to improve code readability:
| let mSnapshot; | |
| let gSnapshot; | |
| // global snapshot includes manifest or module info includes manifest | |
| if (globalRemoteSnapshot) { | |
| if (isManifestProvider(globalRemoteSnapshot)) { | |
| let moduleSnapshot; | |
| let globalSnapshot; | |
| // global snapshot includes manifest or module info includes manifest | |
| if (globalRemoteSnapshot) { | |
| if (isManifestProvider(gl |
| await this.hooks.lifecycle.afterLoadSnapshot.emit({ | ||
| options, | ||
| moduleInfo, | ||
| remoteSnapshot: mSnapshot, | ||
| }); |
There was a problem hiding this comment.
The afterLoadSnapshot hook could benefit from error handling to ensure the system remains in a consistent state even if the hook handlers fail. Consider wrapping the hook emission in a try-catch block:
| await this.hooks.lifecycle.afterLoadSnapshot.emit({ | |
| options, | |
| moduleInfo, | |
| remoteSnapshot: mSnapshot, | |
| }); | |
| try { | |
| await this.hooks.lifecycle.afterLoadSnapshot.emit({ | |
| options, | |
| moduleInfo, | |
| remoteSnapshot: mSnapshot, | |
| }); | |
| } catch (error) { | |
| console.error('Failed to process afterLoadSnapshot hook:', error); | |
| // Consider whether to re-throw or handle the error | |
| } |
packages/runtime/src/core.ts
Outdated
| beforeBridgeRender: new SyncHook<[Record<string, any>], any>(), | ||
| afterBridgeRender: new SyncHook<[Record<string, any>], any>(), | ||
| beforeBridgeDestroy: new SyncHook<[Record<string, any>], any>(), | ||
| afterBridgeDestroy: new SyncHook<[Record<string, any>], any>(), |
There was a problem hiding this comment.
The bridge hook lifecycle methods use a generic Record<string, any> type which is too permissive. Consider creating specific interfaces for each hook's payload to ensure type safety and better documentation. For example:
| beforeBridgeRender: new SyncHook<[Record<string, any>], any>(), | |
| afterBridgeRender: new SyncHook<[Record<string, any>], any>(), | |
| beforeBridgeDestroy: new SyncHook<[Record<string, any>], any>(), | |
| afterBridgeDestroy: new SyncHook<[Record<string, any>], any>(), | |
| beforeBridgeRender: new SyncHook<[BridgeRenderContext], any>(), | |
| afterBridgeRender: new SyncHook<[BridgeRenderResult], any>(), | |
| beforeBridgeDestroy: new SyncHook<[BridgeDestroyContext], any>(), | |
| afterBridgeDestroy: new SyncHook<[BridgeDestroyResult], any>(), |
packages/runtime/src/core.ts
Outdated
| beforeBridgeRender: new SyncHook<[Record<string, any>], any>(), | ||
| afterBridgeRender: new SyncHook<[Record<string, any>], any>(), | ||
| beforeBridgeDestroy: new SyncHook<[Record<string, any>], any>(), | ||
| afterBridgeDestroy: new SyncHook<[Record<string, any>], any>(), |
There was a problem hiding this comment.
The return type 'any' in the SyncHook generic parameters is too loose and could lead to type-safety issues. Consider defining specific return types for each hook to make the API more predictable and maintainable.
For the second code block (lines 291-297):
| this.sharedHandler.hooks, | ||
| this.snapshotHandler.hooks, | ||
| this.loaderHook, | ||
| this.bridgeHook, | ||
| ]); |
There was a problem hiding this comment.
The registerPlugins call is passing hooks as an array which makes the relationship between plugins and hooks implicit. Consider restructuring to use a more explicit mapping of plugins to their corresponding hooks, making the code more maintainable and easier to understand. For example:
| this.sharedHandler.hooks, | |
| this.snapshotHandler.hooks, | |
| this.loaderHook, | |
| this.bridgeHook, | |
| ]); | |
| this.registerPlugins({ | |
| shared: this.sharedHandler.hooks, | |
| snapshot: this.snapshotHandler.hooks, | |
| loader: this.loaderHook, | |
| bridge: this.bridgeHook, | |
| }); |
| initOptions(...args: Parameters<IndexModule.FederationHost['initOptions']>) { | ||
| return this._getInstance().initOptions(...args); | ||
| } |
There was a problem hiding this comment.
The initOptions method could benefit from type safety by explicitly declaring its return type. Based on the pattern seen in other methods, suggest adding the return type:
| initOptions(...args: Parameters<IndexModule.FederationHost['initOptions']>) { | |
| return this._getInstance().initOptions(...args); | |
| } | |
| initOptions(...args: Parameters<IndexModule.FederationHost['initOptions']>): ReturnType<IndexModule.FederationHost['initOptions']> { | |
| return this._getInstance().initOptions(...args); | |
| } |
| get bridgeHook() { | ||
| return this._getInstance().bridgeHook; | ||
| } |
There was a problem hiding this comment.
The bridgeHook getter lacks documentation explaining its purpose and usage. Since this appears to be part of a public API for lifecycle hooks, adding JSDoc would improve maintainability:
| get bridgeHook() { | |
| return this._getInstance().bridgeHook; | |
| } | |
| /** | |
| * Gets the bridge hook instance that handles module federation bridge lifecycle events | |
| * @returns The bridge hook instance | |
| */ | |
| get bridgeHook() { | |
| return this._getInstance().bridgeHook; | |
| } |
There was a problem hiding this comment.
Incremental Review
Comments posted: 4
Configuration
Squadron Mode: essential
Commits Reviewed
cd405919b9dd79637fb73bd57e07d359739d7219...8ff7ef654a77f4388ccb8e29dd95df9d3a447925
Files Reviewed
- packages/bridge/bridge-react/src/provider.tsx
- packages/runtime/src/core.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
| bridgeHook = new PluginSystem({ | ||
| beforeBridgeRender: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| afterBridgeRender: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| beforeBridgeDestroy: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| afterBridgeDestroy: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| }); |
There was a problem hiding this comment.
The bridgeHook initialization could benefit from a dedicated interface to improve code organization and reusability. Extract the hook definitions into a separate interface:
| bridgeHook = new PluginSystem({ | |
| beforeBridgeRender: new SyncHook< | |
| [Record<string, any>], | |
| void | Record<string, any> | |
| >(), | |
| afterBridgeRender: new SyncHook< | |
| [Record<string, any>], | |
| void | Record<string, any> | |
| >(), | |
| beforeBridgeDestroy: new SyncHook< | |
| [Record<string, any>], | |
| void | Record<string, any> | |
| >(), | |
| afterBridgeDestroy: new SyncHook< | |
| [Record<string, any>], | |
| void | Record<string, any> | |
| >(), | |
| }); | |
| interface BridgeHooks { | |
| beforeBridgeRender: SyncHook<[Record<string, any>], void | Record<string, any>>; | |
| afterBridgeRender: SyncHook<[Record<string, any>], void | Record<string, any>>; | |
| beforeBridgeDestroy: SyncHook<[Record<string, any>], void | Record<string, any>>; | |
| afterBridgeDestroy: SyncHook<[Record<string, any>], void | Record<string, any>>; | |
| } | |
| bridgeHook = new PluginSystem<BridgeHooks>({ | |
| beforeBridgeRender: new SyncHook(), | |
| afterBridgeRender: new SyncHook(), | |
| beforeBridgeDestroy: new SyncHook(), | |
| afterBridgeDestroy: new SyncHook(), | |
| }); |
| beforeBridgeRender: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| afterBridgeRender: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| beforeBridgeDestroy: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), | ||
| afterBridgeDestroy: new SyncHook< | ||
| [Record<string, any>], | ||
| void | Record<string, any> | ||
| >(), |
There was a problem hiding this comment.
The return types for the hooks (void | Record<string, any>) are inconsistent with typical lifecycle patterns. Consider standardizing the return types - lifecycle hooks typically either return void (for side effects) or a specific result type. If modification of the context is needed, it should be done through the context parameter rather than the return value.
| @@ -126,6 +126,24 @@ export class FederationHost { | |||
| Promise<(() => Promise<Module>) | undefined> | |||
| >(), | |||
| }); | |||
| bridgeHook = new PluginSystem({ | |||
There was a problem hiding this comment.
The variable name bridgeHook could be more descriptive. Consider renaming to bridgeLifecycle or bridgePluginSystem to better reflect its purpose and functionality.
danpeen
changed the title
There was a problem hiding this comment.
Incremental Review
Comments posted: 1
Configuration
Squadron Mode: essential
Commits Reviewed
8ff7ef654a77f4388ccb8e29dd95df9d3a447925...dfb6d7761330c4fa21e067e8b770d57bc5da9a3f
Files Reviewed
- packages/bridge/vue3-bridge/src/provider.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
| beforeBridgeRender?: (params: RenderFnParams) => void | Record<string, any>; | ||
| afterBridgeRender?: (params: RenderFnParams) => void | Record<string, any>; | ||
| beforeBridgeDestroy?: (params: DestroyParams) => void | Record<string, any>; | ||
| afterBridgeDestroy?: (params: DestroyParams) => void | Record<string, any>; |
There was a problem hiding this comment.
The hook types should be more consistent in their return types. Currently, all hooks allow returning Record<string, any>, but typically only beforeBridgeRender needs to return extra props. Consider:
| beforeBridgeRender?: (params: RenderFnParams) => void | Record<string, any>; | |
| afterBridgeRender?: (params: RenderFnParams) => void | Record<string, any>; | |
| beforeBridgeDestroy?: (params: DestroyParams) => void | Record<string, any>; | |
| afterBridgeDestroy?: (params: DestroyParams) => void | Record<string, any>; | |
| beforeBridgeRender?: (params: RenderFnParams) => void | { extraProps?: Record<string, any> }; | |
| afterBridgeRender?: (params: RenderFnParams) => void; | |
| beforeBridgeDestroy?: (params: DestroyParams) => void; | |
| afterBridgeDestroy?: (params: DestroyParams) => void; |
This makes the API more explicit about which hooks can meaningfully return data.
There was a problem hiding this comment.
Incremental Review
Comments posted: 2
Configuration
Squadron Mode: essential
Commits Reviewed
dfb6d7761330c4fa21e067e8b770d57bc5da9a3f...f2e962492ecaab4795c702beaa18acabeeabd49a
Files Reviewed
- packages/runtime/src/type/plugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
| type ModuleBridgeLifeCycle = Module['host']['bridgeHook']['lifecycle']; | ||
| type ModuleBridgeLifeCycleCyclePartial = Partial<{ | ||
| [k in keyof ModuleBridgeLifeCycle]: Parameters< | ||
| ModuleBridgeLifeCycle[k]['on'] | ||
| >[0]; | ||
| }>; |
There was a problem hiding this comment.
The type definition for ModuleBridgeLifeCycle and ModuleBridgeLifeCycleCyclePartial could be simplified using a helper type to reduce code duplication. Here's a suggested improvement:
| type ModuleBridgeLifeCycle = Module['host']['bridgeHook']['lifecycle']; | |
| type ModuleBridgeLifeCycleCyclePartial = Partial<{ | |
| [k in keyof ModuleBridgeLifeCycle]: Parameters< | |
| ModuleBridgeLifeCycle[k]['on'] | |
| >[0]; | |
| }>; | |
| type CreateLifeCyclePartial<T> = Partial<{ | |
| [K in keyof T]: Parameters<T[K]['on']>[0]; | |
| }>; | |
| type ModuleBridgeLifeCycle = Module['host']['bridgeHook']['lifecycle']; | |
| type ModuleBridgeLifeCycleCyclePartial = CreateLifeCyclePartial<ModuleBridgeLifeCycle>; |
This pattern could be reused for other similar lifecycle type definitions, making the code more maintainable and reducing repetition.
| SnapshotLifeCycleCyclePartial & | ||
| SharedLifeCycleCyclePartial & | ||
| RemoteLifeCycleCyclePartial & | ||
| ModuleLifeCycleCyclePartial & { | ||
| ModuleLifeCycleCyclePartial & | ||
| ModuleBridgeLifeCycleCyclePartial & { | ||
| name: string; | ||
| version?: string; | ||
| }; |
There was a problem hiding this comment.
The plugin interface combines multiple lifecycle partials. Consider adding a descriptive type comment to explain the purpose of each lifecycle component and their relationships. Additionally, the optional version field could benefit from a more specific type if there are expected version formats:
| SnapshotLifeCycleCyclePartial & | |
| SharedLifeCycleCyclePartial & | |
| RemoteLifeCycleCyclePartial & | |
| ModuleLifeCycleCyclePartial & { | |
| ModuleLifeCycleCyclePartial & | |
| ModuleBridgeLifeCycleCyclePartial & { | |
| name: string; | |
| version?: string; | |
| }; | |
| /** | |
| * Plugin configuration interface combining multiple lifecycle hooks: | |
| * - SnapshotLifeCycle: Handles snapshot-related events | |
| * - SharedLifeCycle: Manages shared module events | |
| * - RemoteLifeCycle: Controls remote module loading | |
| * - ModuleLifeCycle: Core module lifecycle events | |
| * - ModuleBridgeLifeCycle: Bridge-specific events | |
| */ | |
| type Plugin = | |
| SnapshotLifeCycleCyclePartial & | |
| SharedLifeCycleCyclePartial & | |
| RemoteLifeCycleCyclePartial & | |
| ModuleLifeCycleCyclePartial & | |
| ModuleBridgeLifeCycleCyclePartial & { | |
| name: string; | |
| version?: `${number}.${number}.${number}`; // Semantic versioning format | |
| }; |
There was a problem hiding this comment.
Incremental Review
Comments posted: 2
Configuration
Squadron Mode: essential
Commits Reviewed
f2e962492ecaab4795c702beaa18acabeeabd49a...b25a91bd26bc94906c08848d36111af48e2a009d
Files Reviewed
- packages/bridge/bridge-react/src/provider.tsx
- packages/bridge/bridge-react/src/remote/index.tsx
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/great-feet-rule.md
- packages/bridge/bridge-react/package.json
- packages/bridge/vue3-bridge/package.json
- pnpm-lock.yaml
| type RenderParams = RenderFnParams & any; | ||
| type DestroyParams = { | ||
| moduleName: string; | ||
| dom: HTMLElement; | ||
| }; |
There was a problem hiding this comment.
The type definitions could be improved for better type safety and consistency:
RenderParamsusing& anydefeats TypeScript's type checking. Define explicit parameters instead.DestroyParamsshould align with the suggestedBaseBridgeParamspattern.
Here's a suggested improvement:
| type RenderParams = RenderFnParams & any; | |
| type DestroyParams = { | |
| moduleName: string; | |
| dom: HTMLElement; | |
| }; | |
| type RenderParams = RenderFnParams & { | |
| // Add specific render parameters here | |
| }; | |
| type DestroyParams = BaseBridgeParams & { | |
| moduleName: string; | |
| }; |
This maintains type safety while providing a consistent parameter structure across the bridge lifecycle hooks.
| providerInfoRef.current?.destroy({ | ||
| moduleName, | ||
| dom: renderDom.current, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The destroy call should handle errors gracefully to prevent potential crashes during cleanup. Consider wrapping it in a try-catch block:
| providerInfoRef.current?.destroy({ | |
| moduleName, | |
| dom: renderDom.current, | |
| }); | |
| } | |
| try { | |
| providerInfoRef.current?.destroy({ | |
| moduleName, | |
| dom: renderDom.current, | |
| }); | |
| } catch (error) { | |
| console.error('Error during bridge component destruction:', error); | |
| } |
This ensures that any errors during cleanup won't propagate up and potentially break the application's unmounting process.
Description
feat: support lifecycyle hooks in module-deferation bridge so that we can support more scene with bridge capability
Related Issue
Types of changes
Checklist