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(bridge-plugin): support disable default alias setting in bridge #3130

Merged
merged 14 commits into from
Nov 5, 2024

Conversation

danpeen
Copy link
Contributor

@danpeen danpeen commented Oct 29, 2024

Description

feat(bridge-plugin): support disable default alias setting in bridge with disableAlias.

In order to avoid problems caused by default alias in some complex scenarios, we support users to disable default alias setting in bridge. After this setting, users can obtain the basename through root component props and display it in the project to set the router's basename.

if you any router issues which is caused by the default alias behavior, try to set disableAlias to not allow the default alias, just config like this:

import { defineConfig } from '@rsbuild/core';
import { pluginReact } from '@rsbuild/plugin-react';
import { pluginModuleFederation } from '@module-federation/rsbuild-plugin';

export default defineConfig({
  plugins: [
    pluginReact(),
    pluginModuleFederation({
      name: 'federation_provider',
      exposes: {
        './button': './src/button.tsx',
      },
      shared: ['react', 'react-dom'],
      // set `disableAlias` to  disable the default alias
      bridge: {
          disableAlias: true,
      },
    }),
  ],
  server: {
    port: 3000,
  },
});

Related Issue

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

Copy link

changeset-bot bot commented Oct 29, 2024

🦋 Changeset detected

Latest commit: 76427c2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 25 packages
Name Type
@module-federation/enhanced Patch
@module-federation/rspack Patch
@module-federation/sdk Patch
@module-federation/modern-js Patch
@module-federation/nextjs-mf Patch
@module-federation/node Patch
@module-federation/rsbuild-plugin Patch
@module-federation/storybook-addon Patch
@module-federation/modernjsapp Patch
@module-federation/devtools Patch
@module-federation/data-prefetch Patch
@module-federation/dts-plugin Patch
@module-federation/esbuild Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/retry-plugin Patch
@module-federation/runtime Patch
@module-federation/utilities Patch
@module-federation/webpack-bundler-runtime Patch
@module-federation/bridge-react-webpack-plugin Patch
@module-federation/bridge-react Patch
@module-federation/bridge-vue3 Patch
@module-federation/runtime-tools Patch
@module-federation/third-party-dts-extractor Patch
@module-federation/bridge-shared Patch

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

Copy link

netlify bot commented Oct 29, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit 76427c2
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/6729baa9679f690008d654da
😎 Deploy Preview https://deploy-preview-3130--module-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Summary

The pull request introduces a new feature that allows users to disable the default alias setting in the bridge for the ModuleFederationPlugin. This is achieved by adding a new option disableAlias to the bridge configuration.

When disableAlias is set, the ReactBridgePlugin will not be applied, allowing users to obtain the basename through the root component props and manually set the router's basename. This can be helpful in complex scenarios where the default alias might cause problems.

The changes are made across three files:

  1. packages/enhanced/src/wrapper/ModuleFederationPlugin.ts: The code changes introduce the disableAlias option and use it to conditionally apply the ReactBridgePlugin.
  2. packages/rspack/src/ModuleFederationPlugin.ts: Similar to the changes in the enhanced package, the code changes introduce the disableAlias option and use it to conditionally apply the ReactBridgePlugin.
  3. packages/sdk/src/types/plugins/ModuleFederationPlugin.ts: The code changes introduce the disableAlias option in the ModuleFederationPluginOptions type definition.

These changes provide users with more flexibility in configuring the ModuleFederationPlugin, allowing them to disable the default alias setting in the bridge when necessary.

File Summaries
File Summary
packages/enhanced/src/wrapper/ModuleFederationPlugin.ts The code changes introduce a new feature to the ModuleFederationPlugin that allows users to disable the default alias setting in the bridge. This is done by adding a new option disableAlias to the bridge configuration, which, when set, will prevent the ReactBridgePlugin from being applied, allowing users to obtain the basename through the root component props and set the router's basename themselves.
packages/rspack/src/ModuleFederationPlugin.ts The code changes introduce a new feature to the ModuleFederationPlugin that allows users to disable the default alias setting in the bridge. This is done by adding a new option disableAlias to the bridge configuration, which is then used to conditionally apply the ReactBridgePlugin.
packages/sdk/src/types/plugins/ModuleFederationPlugin.ts The code changes introduce a new option disableAlias in the bridge configuration of the ModuleFederationPluginOptions. This feature allows users to disable the default alias setting in the bridge, which can be helpful in complex scenarios where the default alias might cause problems. After disabling the alias, users can obtain the basename through the root component props and manually set the router's basename.

Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Incremental Review

Comments posted: 7

Configuration

Squadron Mode: essential

Commits Reviewed

ad605d2c9c1ebd0626ac62af2428485e67779a21...a6859988f0ae2f76502bff04d3471680f439f71d

Files Reviewed
  • packages/enhanced/src/wrapper/ModuleFederationPlugin.ts
  • packages/rspack/src/ModuleFederationPlugin.ts
  • packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored

These files were ignored due to the filter in the squadron.yaml file.

  • apps/router-demo/router-host-2000/rsbuild.config.ts

packages/enhanced/src/wrapper/ModuleFederationPlugin.ts Outdated Show resolved Hide resolved
Comment on lines 49 to 50
'@module-federation/bridge-react',
);
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

packages/rspack/src/ModuleFederationPlugin.ts Outdated Show resolved Hide resolved
packages/rspack/src/ModuleFederationPlugin.ts Show resolved Hide resolved
@@ -127,7 +127,10 @@
);

// 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

packages/sdk/src/types/plugins/ModuleFederationPlugin.ts Outdated Show resolved Hide resolved
@danpeen danpeen changed the title feat: support disable default alias setting in bridge with feat: support disable default alias setting in bridg Oct 29, 2024
@danpeen danpeen changed the title feat: support disable default alias setting in bridg feat: support disable default alias setting in bridge Oct 29, 2024
danpeen and others added 9 commits October 29, 2024 16:41
Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Incremental Review

Comments posted: 2

Configuration

Squadron Mode: essential

Commits Reviewed

f7791881772a7e7dfd21508a13fcb9190e21b6c3...03968535b3c0a29004954207adc1d9829a958f7a

Files Reviewed
  • packages/enhanced/src/wrapper/ModuleFederationPlugin.ts
  • packages/rspack/src/ModuleFederationPlugin.ts
  • packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored

These files were ignored due to the filter in the squadron.yaml file.

  • .changeset/mean-squids-repair.md
  • apps/router-demo/router-host-2000/rsbuild.config.ts

Comment on lines 56 to 58
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
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.

Comment on lines 134 to 136
new ReactBridgePlugin({
moduleFederationOptions: this._options,
}).apply(compiler);
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.

Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Incremental Review

Comments posted: 2

Configuration

Squadron Mode: essential

Commits Reviewed

03968535b3c0a29004954207adc1d9829a958f7a...918c636e0837ba5236d4ba5ad275b11bae3daccf

Files Reviewed
  • packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored

These files were ignored due to the filter in the squadron.yaml file.

  • .changeset/mean-squids-repair.md
  • apps/router-demo/router-host-2000/rsbuild.config.ts

Comment on lines +242 to +243
* Disables the default alias setting in the bridge.
* When true, users must manually handle basename through root component props.
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
*/

Comment on lines 237 to 239
experiments?: {
federationRuntime?: false | 'hoisted';
};
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';
};

@zhoushaw zhoushaw changed the title feat: support disable default alias setting in bridge feat(bridge-plugin): support disable default alias setting in bridge Nov 5, 2024
@zhoushaw zhoushaw merged commit 4eb09e7 into main Nov 5, 2024
19 checks passed
@zhoushaw zhoushaw deleted the feat/bridge-set-alias branch November 5, 2024 07:15
@2heal1 2heal1 mentioned this pull request Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants