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

Jump to bottom

feat(bridge-plugin): support disable default alias setting in bridge #3130

Merged
merged 14 commits into from
Nov 5, 2024
Merged

feat(bridge-plugin): support disable default alias setting in bridge #3130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a685998
feat: support disable default alias setting in bridge with
danpeen Oct 29, 2024
c881aa9
chore: example update reset
danpeen Oct 29, 2024
0694c44
chore: add changeset
danpeen Oct 29, 2024
b004579
chore: merge main
danpeen Oct 30, 2024
9c95487
chore: merge main branch
danpeen Nov 4, 2024
ce35f42
Update packages/rspack/src/ModuleFederationPlugin.ts
danpeen Nov 4, 2024
6342df4
Update packages/enhanced/src/wrapper/ModuleFederationPlugin.ts
danpeen Nov 4, 2024
4c72112
Update packages/enhanced/src/wrapper/ModuleFederationPlugin.ts
danpeen Nov 4, 2024
aa7e3ae
Update packages/rspack/src/ModuleFederationPlugin.ts
danpeen Nov 4, 2024
2172e1c
Update packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
danpeen Nov 4, 2024
0396853
fix: fix build error
danpeen Nov 4, 2024
918c636
chore: fix format check
danpeen Nov 4, 2024
a2573c6
chore: merge main
danpeen Nov 4, 2024
76427c2
chore: merge main branch
danpeen Nov 5, 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/mean-squids-repair.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@module-federation/enhanced': patch
'@module-federation/rspack': patch
'@module-federation/sdk': patch
---

feat: support disable default alias setting in bridge
3 changes: 3 additions & 0 deletions apps/router-demo/router-host-2000/rsbuild.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ export default defineConfig({
path.join(__dirname, './src/runtime-plugin/shared-strategy.ts'),
// path.join(__dirname, './src/runtime-plugin/retry.ts'),
],
// bridge: {
// disableAlias: true,
// },
}),
],
});
5 changes: 4 additions & 1 deletion packages/enhanced/src/wrapper/ModuleFederationPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ export default class ModuleFederationPlugin implements WebpackPluginInstance {
'@module-federation/bridge-react',
);
Comment on lines 49 to 50
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 a comment explaining what the '@module-federation/bridge-react' path represents and why it's being checked. This would improve code maintainability and make the purpose clearer:

Suggested change
'@module-federation/bridge-react',
);
'@module-federation/bridge-react', // Path to React bridge package for module federation

// Check whether react exists
if (fs.existsSync(reactPath)) {
if (
fs.existsSync(reactPath) &&
(!this._options?.bridge || !this._options.bridge.disableAlias)
) {
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
danpeen marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines 56 to 58
Copy link
Contributor

Choose a reason for hiding this comment

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

The ReactBridgePlugin configuration is hardcoded without any validation of the moduleFederationOptions. Consider validating the options before passing them to ensure they meet the expected format:

Suggested change
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
const bridgeConfig = {
moduleFederationOptions: this._options,
};
if (!bridgeConfig.moduleFederationOptions) {
throw new Error('ModuleFederationOptions are required for ReactBridgePlugin');
}
new ReactBridgePlugin(bridgeConfig).apply(compiler);

This adds a basic validation layer to prevent potential runtime issues from invalid configurations.

Expand Down
5 changes: 4 additions & 1 deletion packages/rspack/src/ModuleFederationPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ export class ModuleFederationPlugin implements RspackPluginInstance {
);

// Check whether react exists
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment "Check whether react exists" is not fully accurate as it's checking for both React existence and bridge configuration. A more descriptive comment would be:

Suggested change
// Check whether react exists
// Check for React bridge availability and alias configuration

if (fs.existsSync(reactPath)) {
if (
fs.existsSync(reactPath) &&
(!options?.bridge || !options.bridge.disableAlias)
) {
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
danpeen marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines 134 to 136
Copy link
Contributor

Choose a reason for hiding this comment

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

The ReactBridgePlugin instantiation is done directly without storing the instance. For better plugin lifecycle management and potential cleanup, consider storing the plugin instance:

Suggested change
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
const bridgePlugin = new ReactBridgePlugin({
moduleFederationOptions: this._options,
});
bridgePlugin.apply(compiler);

This approach provides better control over the plugin instance and makes it easier to manage its lifecycle if needed in the future.

Expand Down
8 changes: 8 additions & 0 deletions packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,14 @@ export interface ModuleFederationPluginOptions {
experiments?: {
federationRuntime?: false | 'hoisted';
};
Comment on lines 237 to 239
Copy link
Contributor

Choose a reason for hiding this comment

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

The experiments.federationRuntime property would benefit from JSDoc documentation explaining its purpose and valid values:

Suggested change
experiments?: {
federationRuntime?: false | 'hoisted';
};
/**
* Configuration for experimental features
*/
experiments?: {
/**
* Controls the federation runtime mode
* @default undefined
*/
federationRuntime?: false | 'hoisted';
};

bridge?: {
/**
* Disables the default alias setting in the bridge.
* When true, users must manually handle basename through root component props.
Comment on lines +242 to +243
Copy link
Contributor

Choose a reason for hiding this comment

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

The JSDoc comment is incomplete and cut off mid-sentence. Complete the documentation to properly explain the functionality:

Suggested change
* Disables the default alias setting in the bridge.
* When true, users must manually handle basename through root component props.
* Disables the default alias setting in the bridge.
* When true, users must manually handle basename through root component props.
* @default false
*/

* @default false
*/
disableAlias?: boolean;
};
}
/**
* Modules that should be exposed by this container. Property names are used as public paths.
Expand Down
Loading