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

fix(enhanced): abort the compile process if not find the expose modules #3373

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

2heal1
Copy link
Member

@2heal1 2heal1 commented Dec 18, 2024

Description

When users set invalid expose , the compile will not be aborted , and the remoteEntry will include below contents:

var moduleMap = {
	"./Button": () => {
		var e = new Error("Cannot find module './src/Button.tsx'"); e.code = 'MODULE_NOT_FOUND'; throw e;
	},
};

And the users will not feel the error until the consumer use the error provider . For dynamic provider , this is a bit dangerous , so need to add an option to allow the users feel this error .

So this pr will abort the compile process if not find the expose modules

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 Dec 18, 2024

🦋 Changeset detected

Latest commit: 9bb1a4d

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

This PR includes changesets to release 27 packages
Name Type
@module-federation/error-codes Patch
@module-federation/enhanced Patch
@module-federation/sdk Patch
@module-federation/dts-plugin Patch
@module-federation/runtime Patch
website-new 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/esbuild Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/retry-plugin Patch
@module-federation/rspack 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 Dec 18, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit 9bb1a4d
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/6764d7f833e7df00088ca44d
😎 Deploy Preview https://deploy-preview-3373--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.

@ScriptedAlchemy
Copy link
Member

Should this default to true? Or do we even need an option for this? Can we just fail the build, as this should always be an invalid build?

@2heal1
Copy link
Member Author

2heal1 commented Dec 18, 2024

Should this default to true? Or do we even need an option for this? Can we just fail the build, as this should always be an invalid build?

Ahh yes , i add options because i don't sure whether it can be default behavior . So , we can throw error if exist miss modules by default ?

@ScriptedAlchemy
Copy link
Member

I would vote for this as the default behavior. No options. Like when the entry point is an invalid path. However, if you want to collect feedback, let's default to true. If there are no problems from users, I would suggest we make it ssandard function of the federation plugin in the future: error the build if invalid exposes are passed. Then we can remove the options after user feedback from it.

@2heal1
Copy link
Member Author

2heal1 commented Dec 19, 2024

cool i will set it as default behavior

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

Here is a concise summary of the pull request:

This pull request introduces a new option called abortOnMissExpose to the ModuleFederationPlugin configuration. When enabled, this option will cause the build process to abort if there are any missing exposed modules, rather than silently including them in the remoteEntry file. This helps users catch and address these issues earlier in the development process, improving the overall quality and reliability of the application. The changes span across the ContainerEntryModule.ts and ModuleFederationPlugin.ts files, updating the plugin options interface and the core plugin behavior.

File Summaries
File Summary
packages/enhanced/src/lib/container/ContainerEntryModule.ts The code changes introduce a new option abortOnMissExpose to the ModuleFederationPlugin configuration. This option allows users to abort the compilation process if there are any missing exposed modules, instead of silently including them in the remoteEntry file. This helps users catch and address these issues earlier in the development process.
packages/sdk/src/types/plugins/ModuleFederationPlugin.ts The code changes introduce a new option called abortOnMissExpose to the ModuleFederationPluginOptions interface. This option allows users to configure the behavior of the Module Federation plugin when there are missing exposed modules. When set to true, the plugin will abort the compilation process if any exposed modules are missing, providing users with immediate feedback on the issue.

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: 11

Configuration

Squadron Mode: essential

Commits Reviewed

0166862157ded1b992f1449a764c926ef853449f...b7905dcef79d1e46347298cffbab6bde72f64d9a

Files Reviewed
  • packages/enhanced/src/lib/container/ContainerEntryDependency.ts
  • packages/enhanced/src/lib/container/ContainerEntryModule.ts
  • packages/enhanced/src/lib/container/ContainerEntryModuleFactory.ts
  • packages/enhanced/src/lib/container/ContainerPlugin.ts
  • packages/sdk/src/types/plugins/ContainerPlugin.ts
  • packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored

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

  • .changeset/shaggy-flowers-cry.md
  • apps/website-new/docs/en/guide/troubleshooting/_meta.json
  • apps/website-new/docs/en/guide/troubleshooting/build/BUILD-001.mdx
  • apps/website-new/docs/public/words-map.json
  • apps/website-new/docs/zh/guide/troubleshooting/_meta.json
  • apps/website-new/docs/zh/guide/troubleshooting/build/BUILD-001.mdx
  • packages/enhanced/package.json
  • packages/error-codes/src/desc.ts
  • packages/error-codes/src/error-codes.ts
  • packages/error-codes/src/index.ts
  • pnpm-lock.yaml

@@ -23,6 +23,7 @@ class ContainerEntryDependency extends Dependency {
/** Additional experimental options for container plugin customization */
public experiments: containerPlugin.ContainerPluginOptions['experiments'];
public dataPrefetch: containerPlugin.ContainerPluginOptions['dataPrefetch'];
public abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'];
Copy link
Contributor

Choose a reason for hiding this comment

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

The property name abortOnMissingExposes could be more concise as abortOnMissing since it's already in the context of exposes. This would improve readability while maintaining semantic meaning.

@@ -31,6 +32,7 @@
* @param {string[]} injectRuntimeEntry the path of injectRuntime file.
* @param {containerPlugin.ContainerPluginOptions['experiments']} experiments additional experiments options
* @param {containerPlugin.ContainerPluginOptions['dataPrefetch']} dataPrefetch whether enable dataPrefetch
* @param {containerPlugin.ContainerPluginOptions['abortOnMissingExposes']} abortOnMissingExposes whether abort the compile if miss module
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 description "whether abort the compile if miss module" could be more precise. Suggest: "When true, compilation will abort if any exposed module is missing"

Comment on lines 44 to 42
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'],
) {
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 type validation or a default value for abortOnMissingExposes parameter to ensure predictable behavior. Example:

Suggested change
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'],
) {
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'] = false,

Comment on lines 240 to 254

let str;
if (modules.some((m) => !m.module)) {
if (this._abortOnMissingExposes) {
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
);
process.exit(1);
}
str = runtimeTemplate.throwMissingModuleErrorBlock({
request: modules.map((m) => m.request).join(', '),
});
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 handling for missing modules could be improved. Instead of directly calling process.exit(1), which abruptly terminates the process, consider throwing an error that can be caught and handled gracefully by the build system. This allows for proper cleanup and error reporting.

Suggested change
let str;
if (modules.some((m) => !m.module)) {
if (this._abortOnMissingExposes) {
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
);
process.exit(1);
}
str = runtimeTemplate.throwMissingModuleErrorBlock({
request: modules.map((m) => m.request).join(', '),
});
if (this._abortOnMissingExposes) {
const error = new Error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
})
);
throw error;
}

Comment on lines 66 to 69
private _injectRuntimeEntry: string;
private _experiments: containerPlugin.ContainerPluginOptions['experiments'];
private _dataPrefetch: containerPlugin.ContainerPluginOptions['dataPrefetch'];
private _abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'];
Copy link
Contributor

Choose a reason for hiding this comment

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

The type definition for _abortOnMissingExposes could be more explicit. Since it's a boolean flag, it should be explicitly typed as boolean for better type safety and documentation.

Suggested change
private _injectRuntimeEntry: string;
private _experiments: containerPlugin.ContainerPluginOptions['experiments'];
private _dataPrefetch: containerPlugin.ContainerPluginOptions['dataPrefetch'];
private _abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'];
private _abortOnMissingExposes: boolean;

Comment on lines 40 to 43
dep.injectRuntimeEntry,
dep.experiments,
dep.dataPrefetch,
dep.abortOnMissingExposes,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

The code adds a new parameter abortOnMissingExposes to handle missing expose modules, which is a good addition for error detection. However, the code could benefit from a type assertion or validation before using this value. Consider adding a runtime check:

Suggested change
dep.injectRuntimeEntry,
dep.experiments,
dep.dataPrefetch,
dep.abortOnMissingExposes,
),
dep.injectRuntimeEntry,
dep.experiments,
dep.dataPrefetch,
typeof dep.abortOnMissingExposes === 'boolean' ? dep.abortOnMissingExposes : false,

This ensures a default behavior and type safety even if the value is undefined or of incorrect type.

@@ -103,6 +103,10 @@ export interface ContainerPluginOptions {
federationRuntime?: false | 'hoisted';
};
dataPrefetch?: DataPrefetch;
/**
* If not find exposes module , it will throw error when abortOnMissingExposes is true
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 should be more precise. Consider: "When true, compilation will abort if any exposed module cannot be found"

/**
* If not find exposes module , it will throw error when abortOnMissingExposes is true
*/
abortOnMissingExposes?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider providing a default value in the comment to help users understand the default behavior. Example: ```suggestion
abortOnMissingExposes?: boolean; // Defaults to false

Comment on lines 233 to 234
* If not find exposes module , it will throw error when abortOnMissingExposes is true
*/
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 could be more descriptive and include an example. Consider expanding it to better explain the purpose and consequences:

Suggested change
* If not find exposes module , it will throw error when abortOnMissingExposes is true
*/
/**
* When true, compilation will fail if any exposed module specified in the configuration cannot be found.
* This helps catch configuration errors early in the build process rather than at runtime.
* @example
* abortOnMissingExposes: true // Will throw error if './src/Button.tsx' doesn't exist
*/

/**
* If not find exposes module , it will throw error when abortOnMissingExposes is true
*/
abortOnMissingExposes?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a more descriptive name for better clarity. 'abortOnMissingExposes' could be renamed to 'failOnMissingExports' or 'strictExports' to better reflect its purpose:

Suggested change
abortOnMissingExposes?: boolean;
strictExports?: boolean;

@2heal1 2heal1 force-pushed the fix/abort-on-miss-expose branch from da05996 to 9bb1a4d Compare December 20, 2024 02:35
@2heal1 2heal1 changed the title fix(enhanced): add abortOnMissExpose field fix(enhanced): abort the compile process if not find the expose modules Dec 20, 2024
@2heal1
Copy link
Member Author

2heal1 commented Dec 20, 2024

@ScriptedAlchemy I change the pr , and now it will abort the compile process if not find the expose modules

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: 3

Configuration

Squadron Mode: essential

Commits Reviewed

40a0884bef6baf213fcf980ad08bff8b0f44969a...9bb1a4d39fe039de628e766996da4b0361fbb933

Files Reviewed
  • packages/enhanced/src/lib/container/ContainerEntryModule.ts
Files Ignored

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

  • .changeset/shaggy-flowers-cry.md
  • apps/website-new/docs/en/guide/troubleshooting/_meta.json
  • apps/website-new/docs/en/guide/troubleshooting/build/BUILD-001.mdx
  • apps/website-new/docs/public/words-map.json
  • apps/website-new/docs/zh/guide/troubleshooting/_meta.json
  • apps/website-new/docs/zh/guide/troubleshooting/build/BUILD-001.mdx
  • packages/enhanced/package.json
  • packages/error-codes/src/desc.ts
  • packages/error-codes/src/error-codes.ts
  • packages/error-codes/src/index.ts
  • pnpm-lock.yaml

Comment on lines 237 to +244
if (modules.some((m) => !m.module)) {
str = runtimeTemplate.throwMissingModuleErrorBlock({
request: modules.map((m) => m.request).join(', '),
});
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
);
process.exit(1);
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 handling logic should be extracted into a separate method for better code organization and reusability. Additionally, the error message construction could be moved to a dedicated function. Here's a suggested refactoring:

Suggested change
if (modules.some((m) => !m.module)) {
str = runtimeTemplate.throwMissingModuleErrorBlock({
request: modules.map((m) => m.request).join(', '),
});
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
);
process.exit(1);
if (modules.some((m) => !m.module)) {
const missingModules = modules.filter((m) => !m.module);
this._handleMissingModules(missingModules);
} else {
str = `return ${runtimeTemplate.blockPromise({
block,

Then add a private method:

==CODE_SUGGESTION==
private _handleMissingModules(missingModules: Array<{name: string, module: any, request: string}>) {
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: missingModules,
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
})
);
process.exit(1);
}
==CODE_SUGGESTION==

@@ -229,9 +235,13 @@

let str;
if (modules.some((m) => !m.module)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The condition modules.some((m) => !m.module) could be made more explicit with a descriptive variable name to improve code readability:

Suggested change
if (modules.some((m) => !m.module)) {
const hasMissingModules = modules.some((m) => !m.module);
if (hasMissingModules) {

Comment on lines +239 to +242
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
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 message construction could be simplified by using object destructuring and making the environment variable access more robust:

Suggested change
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter((m) => !m.module),
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'],
}),
const federationPath = process.env['FEDERATION_WEBPACK_PATH'] || '';
logger.error(
getShortErrorMsg(BUILD_001, buildDescMap, {
exposeModules: modules.filter(m => !m.module),
FEDERATION_WEBPACK_PATH: federationPath,
})
);

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