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: esm files should use .mjs extention #3159

Merged
merged 10 commits into from
Nov 5, 2024
Merged

fix: esm files should use .mjs extention #3159

merged 10 commits into from
Nov 5, 2024

Conversation

ScriptedAlchemy
Copy link
Member

@ScriptedAlchemy ScriptedAlchemy commented Nov 2, 2024

Description

make esm files emit with .mjs extention

Related Issue

module-federation/vite#163 (comment)

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 Nov 2, 2024

🦋 Changeset detected

Latest commit: de79650

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/webpack-bundler-runtime Patch
@module-federation/data-prefetch Patch
@module-federation/runtime-tools Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/runtime Patch
@module-federation/sdk Patch
@module-federation/enhanced Patch
@module-federation/nextjs-mf Patch
@module-federation/rspack Patch
@module-federation/dts-plugin Patch
@module-federation/modern-js Patch
@module-federation/devtools Patch
@module-federation/node Patch
@module-federation/retry-plugin Patch
@module-federation/esbuild Patch
@module-federation/rsbuild-plugin Patch
@module-federation/storybook-addon Patch
@module-federation/utilities Patch
@module-federation/bridge-react-webpack-plugin Patch
@module-federation/bridge-react Patch
@module-federation/bridge-vue3 Patch
@module-federation/modernjsapp 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 Nov 2, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit de79650
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/6729968db0b7bd0008b43bd0
😎 Deploy Preview https://deploy-preview-3159--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 core changes in this pull request focus on updating the file extensions used for ESM (ECMAScript Module) files in the module federation codebase. The changes ensure that ESM files use the correct .mjs extension, which addresses an issue where the incorrect .cjs extension was being used.

The changes include:

  • Updating the file extension for ESM files in the FederationRuntimePlugin.ts file from .cjs to .mjs.
  • Changing the import path for the @module-federation/runtime package to use the .mjs file extension instead of .esm.js in the ModuleFederationPlugin.ts file.

These changes help to improve the integration of the module federation functionality with the existing codebase and ensure that ESM files are properly recognized and used in the module federation runtime.

File Summaries
File Summary
packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts The code changes update the file extension for ESM (ECMAScript Module) files from .cjs to .mjs. This ensures that the ESM files are properly identified and used in the module federation runtime, addressing an issue where the incorrect file extension was being used.
packages/rspack/src/ModuleFederationPlugin.ts The changes in this file update the import path for the @module-federation/runtime package to use the .mjs file extension instead of .esm.js. This ensures that the ESM (ECMAScript Module) files are properly recognized and used in the module federation setup.

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

680fc9e06e962ea988525a2ce4a5c9bff92ed08b...7715b994791ebb4b4cd2f9ed70781f9deeb4a02b

Files Reviewed
  • packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
  • packages/rspack/src/ModuleFederationPlugin.ts
Files Ignored

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

  • .changeset/popular-pillows-draw.md
  • packages/data-prefetch/package.json
  • packages/data-prefetch/rollup.config.js
  • packages/data-prefetch/src/cli/index.ts
  • packages/managers/rollup.config.js
  • packages/manifest/rollup.config.js
  • packages/runtime-tools/package.json
  • packages/runtime-tools/rollup.config.js
  • packages/runtime/package.json
  • packages/runtime/rollup.config.js
  • packages/sdk/package.json
  • packages/sdk/rollup.config.js
  • packages/webpack-bundler-runtime/package.json
  • packages/webpack-bundler-runtime/rollup.config.js
  • pnpm-lock.yaml

ScriptedAlchemy and others added 2 commits November 4, 2024 14:53
# Conflicts:
#	packages/data-prefetch/rollup.config.js
#	packages/runtime-tools/rollup.config.js
#	packages/runtime/rollup.config.js
#	packages/sdk/rollup.config.js
#	packages/webpack-bundler-runtime/rollup.config.js
#	pnpm-lock.yaml
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: 4

Configuration

Squadron Mode: essential

Commits Reviewed

083af4ba6aa3c563267fe0a17f846a86ccde4476...c56143fc51bd3c2cec1a37f6d6ea2a13036a2783

Files Reviewed
  • packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
  • packages/rspack/src/ModuleFederationPlugin.ts
Files Ignored

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

  • .changeset/popular-pillows-draw.md
  • packages/data-prefetch/package.json
  • packages/data-prefetch/rollup.config.js
  • packages/data-prefetch/src/cli/index.ts
  • packages/runtime-tools/package.json
  • packages/runtime-tools/rollup.config.js
  • packages/runtime/package.json
  • packages/runtime/rollup.config.js
  • packages/sdk/package.json
  • packages/sdk/rollup.config.js
  • packages/webpack-bundler-runtime/package.json
  • packages/webpack-bundler-runtime/rollup.config.js

@module-federation module-federation deleted a comment from squadronai bot Nov 5, 2024
@module-federation module-federation deleted a comment from squadronai bot Nov 5, 2024
@module-federation module-federation deleted a comment from squadronai bot Nov 5, 2024
@module-federation module-federation deleted a comment from squadronai bot Nov 5, 2024
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: 4

Configuration

Squadron Mode: essential

Commits Reviewed

0b44ffae88fd261128629a015d63c65f3699da5a...de79650bd174c22b3245b02186461a799a72b772

Files Reviewed
  • packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
  • packages/rspack/src/ModuleFederationPlugin.ts
Files Ignored

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

  • .changeset/popular-pillows-draw.md
  • packages/data-prefetch/package.json
  • packages/data-prefetch/rollup.config.js
  • packages/data-prefetch/src/cli/index.ts
  • packages/runtime-tools/package.json
  • packages/runtime-tools/rollup.config.js
  • packages/runtime/package.json
  • packages/runtime/rollup.config.js
  • packages/sdk/package.json
  • packages/sdk/rollup.config.js
  • packages/webpack-bundler-runtime/package.json
  • packages/webpack-bundler-runtime/rollup.config.js

Comment on lines 373 to +374
if (isHoisted) {
runtimePath = runtimePath.replace('.cjs', '.esm');
runtimePath = runtimePath.replace('.cjs.js', '.esm.mjs');
Copy link
Contributor

Choose a reason for hiding this comment

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

The string replacement for file extension is brittle and could fail if the path doesn't contain '.cjs.js'. A more robust approach would be:

Suggested change
if (isHoisted) {
runtimePath = runtimePath.replace('.cjs', '.esm');
runtimePath = runtimePath.replace('.cjs.js', '.esm.mjs');
runtimePath = runtimePath.replace(/\.cjs\.js$/, '.esm.mjs');

This ensures we only replace at the end of the string and handles cases where '.cjs.js' might appear elsewhere in the path.

Comment on lines 439 to 443
if (this.options?.experiments?.federationRuntime === 'hoisted') {
this.bundlerRuntimePath = this.bundlerRuntimePath.replace(
'.cjs.js',
'.esm.js',
'.esm.mjs',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the previous suggestion, the extension replacement here should be more precise:

Suggested change
if (this.options?.experiments?.federationRuntime === 'hoisted') {
this.bundlerRuntimePath = this.bundlerRuntimePath.replace(
'.cjs.js',
'.esm.js',
'.esm.mjs',
);
this.bundlerRuntimePath = this.bundlerRuntimePath.replace(
/\.cjs\.js$/,
'.esm.mjs',
);

Comment on lines 452 to +453
if (/webpack-bundler-runtime/.test(resolveData.contextInfo.issuer)) {
resolveData.request = RuntimePath.replace('cjs', 'esm');
resolveData.request = RuntimePath.replace('cjs.js', 'esm.mjs');
Copy link
Contributor

Choose a reason for hiding this comment

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

The extension replacement here is also brittle. Consider using a more robust approach:

Suggested change
if (/webpack-bundler-runtime/.test(resolveData.contextInfo.issuer)) {
resolveData.request = RuntimePath.replace('cjs', 'esm');
resolveData.request = RuntimePath.replace('cjs.js', 'esm.mjs');
resolveData.request = RuntimePath.replace(/\.cjs\.js$/, '.esm.mjs');

Comment on lines 101 to 104
const runtimeESMPath = require.resolve(
'@module-federation/runtime/dist/index.esm.js',
'@module-federation/runtime/dist/index.esm.mjs',
{ paths: [options.implementation] },
);
Copy link
Contributor

Choose a reason for hiding this comment

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

The path resolution for the ESM runtime file correctly uses the .mjs extension, which aligns with the Node.js ESM specification. However, to make the code more resilient to potential path resolution issues, consider adding error handling:

Suggested change
const runtimeESMPath = require.resolve(
'@module-federation/runtime/dist/index.esm.js',
'@module-federation/runtime/dist/index.esm.mjs',
{ paths: [options.implementation] },
);
const runtimeESMPath = (() => {
try {
return require.resolve(
'@module-federation/runtime/dist/index.esm.mjs',
{ paths: [options.implementation] },
);
} catch (error) {
throw new Error(`Failed to resolve ESM runtime path: ${error.message}`);
}
})();

This change adds proper error handling and provides a more descriptive error message if the runtime file cannot be resolved, which can help with debugging deployment issues.

@ScriptedAlchemy ScriptedAlchemy merged commit 206b56d into main Nov 5, 2024
17 checks passed
@ScriptedAlchemy ScriptedAlchemy deleted the output-mjs branch November 5, 2024 04:08
@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