(BREAKING) Change Webpack config to output real ESM modules #1393
+126
−86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ibacher
changed the title
Contributor
|
Size Change: -4.85 MB (-45.18%) 🎉 Total Size: 5.88 MB
ℹ️ View Unchanged
|
ibacher
commented
Jun 11, 2025
| timeout && clearTimeout(timeout); | ||
|
|
||
| const jsPackageSlug = slugify(jsPackage); | ||
| const maybeContainer = await import(/* webpackIgnore: true */ jsPackage); |
Member
Author
There was a problem hiding this comment.
Once we've migrated the existing apps, we should be able to replace this whole dynamic loading stuff with something like:
const container = await import(/* webpackIgnore: true */ jsPackage);
if (!isFederatedModule(container)) {
const error = `The package ${jsPackage} is not a federated module`;
console.error(error);
throw new Error(error);
}
container.init(__webpack_share_scopes__.default);
const factory = await container.get(share);
const module = factory();
if (!(typeof module === 'object') || module === null) {
const error = `Container for ${jsPackage} did not return an ESM module as expected`;
console.error(error);
throw new Error(error);
}
return module as unknown as T;And once we get to shared dependencies on the import map, this will all go away completely.
ibacher
commented
Jun 11, 2025
Member
Author
There was a problem hiding this comment.
I've largely left the app shell itself alone. When I tried moving it to an ESM, the window.initializeSpa variable wasn't being set. We'd also need to migrate to moving at least the import-map-overrides and import-map-injector inclusions to the index.ejs script.
Not exactly sure how to get those imports into the ejs file so that they use the local copy.
It may be that we need to switch the script block to a module, import @openmrs/esm-app-shell and run the initializeSpa() entry point from there?
Member
Author
There was a problem hiding this comment.
@jolyndenning This is a point I could use some pointers on.
There was a problem hiding this comment.
I think the following would fix the initializeSpa problem:
<script>
window.importMapInjector.initPromise
.then(() => import('@openmrs/whichever-module-defines-initializeSpa'))
.then(module => {
module.initializeSpa({...});
})
.catch(err => {
// some kind of error handling?
})
</script>
Member
|
Thanks, @ibacher. Did you mean to use the BREAKING conventional commit label here? |
Member
Author
|
Actually, no. I called the branch breaking because I thought it would be, but this should actually be backwards-compatible (once I fix whatever is causing the failures there). |
ibacher
commented
Jun 11, 2025
| proxyRes.headers && delete proxyRes.headers['content-security-policy']; | ||
|
|
||
| if (req.url.endsWith('importmap.json')) { | ||
| proxyRes.headers['content-type'] = 'application/importmap+json'; |
Member
Author
There was a problem hiding this comment.
Mostly a note to myself, but we'll need to make a similar change in the CLI, the RefApp, and the SPA module
internettrans
left a comment
internettrans
left a comment
There was a problem hiding this comment.
Will the feat() PR title result in this being applied as a minor rather than major change?
The branch name is breaking/use-real-esms
| /** | ||
| * Globals added by the @single-spa/import-map-injector package. | ||
| */ | ||
| importMapInjector: { |
There was a problem hiding this comment.
This is defined in the types published to npm. Is it possible to reuse those types?
https://app.unpkg.com/@single-spa/[email protected]/files/types/import-map-injector.d.ts#L11
| <% } %> | ||
| <% if (openmrsCoreImportmap) { %> | ||
| <script type="systemjs-importmap"><%= openmrsCoreImportmap %></script> | ||
| <script type="injector-importmap"><%= openmrsCoreImportmap %></script> |
There was a problem hiding this comment.
why spaces added? For indentation because of ejs? Output html will have indentation bc of this?
Member
Author
There was a problem hiding this comment.
Mostly for consistency. It matches the indentation of all the other sub-elements of <head>.
There was a problem hiding this comment.
I think the following would fix the initializeSpa problem:
<script>
window.importMapInjector.initPromise
.then(() => import('@openmrs/whichever-module-defines-initializeSpa'))
.then(module => {
module.initializeSpa({...});
})
.catch(err => {
// some kind of error handling?
})
</script>| const { run } = await import(/* webpackPreload: true */ './run'); | ||
| return run(configUrls); | ||
| }); | ||
| return window.importMapInjector.initPromise.then(() => |
There was a problem hiding this comment.
How is esm-app-shell built and loaded into the html file? What is the module format of the built version of it?
Is it part of htmlWebpackPlugin's body tags? <%= htmlWebpackPlugin.tags.bodyTags %>?
What I would expect is to see a script tag only loading import-map-injector within index.ejs, and then it loads esm-app-shell after initPromise resolves. The reason is that native import() or <script type=module> cannot be used until the import map is installed.
In index.ejs, it looks like sometimes there is an external import map and sometimes not. When there is not an external import map, waiting for initPromise is not required, since import-map-injector inserts the native import map synchronously. The optimal solution might account for that, with two code paths for external vs inline import maps.
Member
Author
There was a problem hiding this comment.
Is it part of htmlWebpackPlugin's body tags?
Yes.
What I would expect is to see a script tag only loading import-map-injector within index.ejs
For now because I didn't move the app shell entry point to an ESM, it wasn't necessary, but yes.
In index.ejs, it looks like sometimes there is an external import map and sometimes not.
We could take that into account, but presently for production builds, it's always using an external import map; the embedded import map is currently only used in development mode.
There was a problem hiding this comment.
I prefer having the app shell be an ES module so that it can export things that are available via cross microfrontend import to other microfrontends.
| }, | ||
| mode, | ||
| devtool: isProd ? 'hidden-nosources-source-map' : 'eval-source-map', | ||
| devtool: isProd ? 'hidden-nosources-source-map' : 'source-map', |
There was a problem hiding this comment.
Why this change? Since eval-source-map is for dev?
Member
Author
There was a problem hiding this comment.
Ah! This shouldn't've been part of the commit. I was just looking through the generated file and it's easier to read with the source map externalized.
Member
Author
Well, we version things manually, but as-is, this is currently backwards-compatible with existing code. |
Technically this is a breaking change, depending on the contents of the bundles being loaded, since modules loaded via However, practically speaking, that change might not break things, depending on the implementation details of the bundles. Another way it could be breaking is because the global variables previously created are no longer created. Is there other code outside of It's safer to mark this as a break change, and technically it is one. |
Member
Author
That and |
ibacher
changed the title
Contributor
|
@ibacher Thanks for working on this issue. Could you kindly fix the merge conflicts, please? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
One goal for O3 has been to support "real" ES modules—after all, we've named most of our projects
esm-*as if that's the case. This PR is a small step towards realizing that goal.This changes the Webpack and Rspack configurations to output using the "module" format instead of the "var" format, meaning that package entry points, at least, are ESMs. I've also patched the dynamic loading code to first try loading a MFE as a ES module and only fall back to the old process for code that cannot be imported this way.
Screenshots
Related Issue
Other
The medium-term plan is to drop support for module federation and switch to using shared libraries on the import map. Those steps will necessarily be breaking changes.