-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9bb1a4d 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. |
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 ? |
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. |
cool i will set it as default behavior |
There was a problem hiding this 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. |
There was a problem hiding this 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']; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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"
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'], | ||
) { |
There was a problem hiding this comment.
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:
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'], | |
) { | |
abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes'] = false, |
|
||
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(', '), | ||
}); |
There was a problem hiding this comment.
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.
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; | |
} |
private _injectRuntimeEntry: string; | ||
private _experiments: containerPlugin.ContainerPluginOptions['experiments']; | ||
private _dataPrefetch: containerPlugin.ContainerPluginOptions['dataPrefetch']; | ||
private _abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes']; |
There was a problem hiding this comment.
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.
private _injectRuntimeEntry: string; | |
private _experiments: containerPlugin.ContainerPluginOptions['experiments']; | |
private _dataPrefetch: containerPlugin.ContainerPluginOptions['dataPrefetch']; | |
private _abortOnMissingExposes: containerPlugin.ContainerPluginOptions['abortOnMissingExposes']; | |
private _abortOnMissingExposes: boolean; |
dep.injectRuntimeEntry, | ||
dep.experiments, | ||
dep.dataPrefetch, | ||
dep.abortOnMissingExposes, | ||
), |
There was a problem hiding this comment.
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:
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
* If not find exposes module , it will throw error when abortOnMissingExposes is true | ||
*/ |
There was a problem hiding this comment.
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:
* 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; |
There was a problem hiding this comment.
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:
abortOnMissingExposes?: boolean; | |
strictExports?: boolean; |
da05996
to
9bb1a4d
Compare
@ScriptedAlchemy I change the pr , and now it will abort the compile process if not find the expose modules |
There was a problem hiding this 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
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); |
There was a problem hiding this comment.
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:
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)) { |
There was a problem hiding this comment.
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:
if (modules.some((m) => !m.module)) { | |
const hasMissingModules = modules.some((m) => !m.module); | |
if (hasMissingModules) { |
getShortErrorMsg(BUILD_001, buildDescMap, { | ||
exposeModules: modules.filter((m) => !m.module), | ||
FEDERATION_WEBPACK_PATH: process.env['FEDERATION_WEBPACK_PATH'], | ||
}), |
There was a problem hiding this comment.
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:
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, | |
}) | |
); |
Description
When users set invalid expose , the compile will not be aborted , and the remoteEntry will include below contents:
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
Checklist