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

Don't used named exports when exporting in ES Module format. #531

Closed
wants to merge 1 commit into from

Conversation

manbearwiz
Copy link

@manbearwiz manbearwiz commented Nov 25, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Currently, setting "libraryTarget": "module" emits the following error:

Error: Library name must be unset. Common configuration options that specific library names are 'output.library[.name]', 'entry.xyz.library[.name]', 'ModuleFederationPlugin.name' and 'ModuleFederationPlugin.library[.name]'.

What is the new behavior?

The error is no longer emitted.

Does this PR introduce a breaking change?

  • No

@arturovt
Copy link
Member

Tests are failing. Could you have a look?

@manbearwiz
Copy link
Author

I noticed the failing e2e tests when I first made the PR. Those tests run inconsistently on my machine for main so I assumed this was an unrelated issue. Have you seen this before? I only ask because this change really only effect projects with "libraryTarget": "module" which doesn't include the e2e test projects.

@jolyndenning
Copy link
Member

Why is the build failing? I would expect the following additional changes to be made to the shared webpack config:

  1. experiments.outputModule set to true
  2. devServer.hot set to false (see HookWebpackError: HMR is not implemented for module chunk format yet [webpack 5] webpack/webpack#17636)
  3. Remove systemjs-webpack-interop plugin, if it's present
  4. Set output.publicPath to "auto"
  5. Provide some kind of way for single-spa-angular users to toggle between systemjs and esm output, so that they can stay on latest versions of single-spa-angular and upgrade to native modules when the root config is ready for it. Perhaps in angular.json, package.json, or some other config. It has been a long time since I've worked on this project so I don't know the best way to pass a configuration option to the plugin. In webpack-config-single-spa and vue-cli-plugin-single-spa, the name of the configuration option is outputSystemJS and it defaults to false, so the same naming convention might be best for single-spa-angular.

@manbearwiz
Copy link
Author

Why is the build failing?

I'm not sure. yarn test:ci:integration fails on the master branch for me on macos and windows. Would you be able to confirm that latest master passes on the CI runner? All unit tests that touch this code do pass.

I would expect ..

  1. experiments.outputModule set to true

Yes, this is needed to support ESM. Since this would be my first contribution in this repo, my goal wasn't to do everything at once, but rather do it as a few minimal, easily tested and reviewable non-breaking changes. Because output.library should be removed for anyone setting output.libraryTarget: 'module', it seemed like a good place to start.

  1. devServer.hot set to false

I'm not sure on this one. I didn't run into this error.

  1. Remove systemjs-webpack-interop plugin, if it's present

Yeah that definitely needs to go away in the long term. I have played around with updating systemjs-webpack-interop to log a warning and fail gracefully when window.System.resolve is undefined, just to smooth out the transition and not require code changes to every angular app in the short term.

  1. Set output.publicPath to "auto"

Yes.

  1. Provide some kind of way for single-spa-angular users to toggle between systemjs and esm output

I think the best place for this is the existing customWebpackConfig.libraryTarget in the angular.json. By default, this is currently set to umd on new single-spa-angular apps so updating it to module would be very straightforward.

If you want to make these changes yourself, feel free to close this, or if you know which of the above items you would like me to include in this PR, just lmk.

@manbearwiz
Copy link
Author

@joeldenning @arturovt I looked into the failing tests more. The stack traces link to https://cdn.jsdelivr.net/npm/@esm-bundle/angular/system/es2022/angular-core.min.js which resolves to @angular/[email protected]. My guess is that when ng19 was released and those cdns updated they broke the e2e tests.

@jolyndenning
Copy link
Member

jolyndenning commented Feb 6, 2025

I hope to work on an official angular-microfrontends example soon and will likely make the necessary modifications to single-spa-angular as part of it. Using libraryTarget in the custom webpack config as a single-spa-angular configuration option doesn't sound like the most maintainable long-term approach. The webpack changes I suggested in my prior comment are after implementing native modules for both react and vue. Rather than debate them, I am closing this issue and will work on native modules feature on my own. I am stalked by people who try to prove me wrong, so it's easier just to do the work myself rather than let a contributor debate the workings of webpack that I've verified. The devServer.hot option can't be enabled when using libraryTarget module because the feature hasn't yet been implemented, but I am not familiar enough with the angular webpack config to know if hmr is ever enabled or if angular has a non-standard hmr implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants