Skip to content

fix: normalize bundler runtime import paths #1997

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

Merged
merged 4 commits into from
Feb 1, 2024
Merged

fix: normalize bundler runtime import paths #1997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2c79d9
fix: normalize bundler runtime import paths
jonthomp Jan 19, 2024
55d5480
Merge branch 'module-federation:main' into fix/normalize-bundler-runt…
jonthomp Jan 19, 2024
d2b1d7e
fix: rejoin webpack path so it includes drive and path on windows
jonthomp Jan 22, 2024
18dad3d
Merge branch 'main' into fix/normalize-bundler-runtime-import-paths
ScriptedAlchemy Feb 1, 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
5 changes: 5 additions & 0 deletions .changeset/soft-days-chew.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@module-federation/enhanced': patch
---

normalize bundler runtime import paths
14 changes: 10 additions & 4 deletions packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,29 @@ class FederationRuntimePlugin {

static getTemplate(runtimePlugins: string[], bundlerRuntimePath?: string) {
// internal runtime plugin
const normalizedBundlerRuntimePath = (
bundlerRuntimePath || BundlerRuntimePath
).replaceAll('\\', '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use path.normalize here?

Copy link
Member

Choose a reason for hiding this comment

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

yeah can we look at that? otherwise theres also a normalize webpack path function in this codebase it think, for us to resolve the right webpack.

@2heal1 is that right or should we implement this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did try a path.normalize in the normalizWebpackPath util but it apparently was not working, I'll have another look

Copy link
Contributor Author

@jonthomp jonthomp Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've just double checked, normalizeWebpackPath isn't used for these paths, they're @module-federation modules not webpack.

path.normalize considers the path as already being valid so makes no changes. We tried some different methods but this is the only way we could generate paths that also work on Windows

Copy link
Member

Choose a reason for hiding this comment

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

gotcha

Copy link
Member

Choose a reason for hiding this comment

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

yes, i miss to test the function in Windows, much appreciated for modifying this @jonthomp !

And i need to test the modify can be worked as expect in our codebase. I will test it today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@2heal1 happy to help!

Sorry I also missed the remaining Windows fix that we have in a patch. I have now added it to this PR.

On Windows, the line webpackLocationWithDetail.split(':').slice(0, -2)[0] returns C because the path is split into ['C', 'some\path\to\webpack']


let runtimePluginTemplates = '';
const runtimePLuginNames: string[] = [];

if (Array.isArray(runtimePlugins)) {
runtimePlugins.forEach((runtimePlugin, index) => {
const runtimePluginName = `plugin_${index}`;
const runtimePluginPath = path.isAbsolute(runtimePlugin)
? runtimePlugin
: path.join(process.cwd(), runtimePlugin);
const runtimePluginPath = (
path.isAbsolute(runtimePlugin)
? runtimePlugin
: path.join(process.cwd(), runtimePlugin)
).replaceAll('\\', '/');

runtimePluginTemplates += `import ${runtimePluginName} from '${runtimePluginPath}';\n`;
runtimePLuginNames.push(runtimePluginName);
});
}

return Template.asString([
`import federation from '${bundlerRuntimePath || BundlerRuntimePath}';`,
`import federation from '${normalizedBundlerRuntimePath}';`,
runtimePluginTemplates,
`${federationGlobal} = {...federation,...${federationGlobal}};`,
`if(!${federationGlobal}.instance){`,
Expand Down
5 changes: 4 additions & 1 deletion packages/sdk/src/normalize-webpack-path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ export function getWebpackPath(
const webpackLocationWithDetail = webpackErrLocation
.replace(/[^\(\)]+/, '')
.slice(1, -1);
const webpackPath = webpackLocationWithDetail.split(':').slice(0, -2)[0];
const webpackPath = webpackLocationWithDetail
.split(':')
.slice(0, -2)
.join(':');
if (options?.framework === 'nextjs') {
if (webpackPath.endsWith('webpack.js')) {
return webpackPath.replace('webpack.js', 'index.js');
Expand Down