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

consider publshing an ESM bundle #182

Closed
donbowman opened this issue Jul 16, 2020 · 4 comments
Closed

consider publshing an ESM bundle #182

donbowman opened this issue Jul 16, 2020 · 4 comments

Comments

@donbowman
Copy link

donbowman commented Jul 16, 2020

Angular 10 now warns when CommonJS or AMD are present:

WARNING in /home/don/src-ag/platform/portal/node_modules/agilicus-angular/__ivy_ngcc__/fesm2015/agilicus-angular.js depends on 'yaml'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

Consider publishing an ESM bundle to avoid this issue. Possibly using https://github.com/esm-bundle/

See also https://stackoverflow.com/questions/62592903/upgrading-to-angular-10-fix-commonjs-or-amd-dependencies-can-cause-optimizatio

@eemeli
Copy link
Owner

eemeli commented Jul 16, 2020

See #138 for prior discussion on this.

Effectively, the current packaging is as efficient as it can be for a complete YAML library, and switching to a completely-ES module build would not bring additional savings. Being able to drop features at build-time would require some significant refactoring, and probably necessitate separate custom builds for any given feature set, or complicate the current API significantly.

Some of my 2.0 work has made that more possible, e.g. eventually offering an endpoint that offered YAML.parse() & YAML.stringify() without YAML 1.1 support, and I'd welcome help with that, but it's not a matter of just repackaging the library.

@donbowman
Copy link
Author

would you be open to the https://github.com/esm-bundle/ repackaging option? It makes a @esm-bundle/yaml avail if so. it then automatically builds as you publish i think.

else i guess we can just add a workaround for the warning in angular, but my issue is i'm publishing an angular library package which has this as peerDependency, so my end users get the warning too.

@eemeli
Copy link
Owner

eemeli commented Jul 19, 2020

That seems like a rather hacky solution for Angular's false positive warning. It may get rid of it, but it'll mean that any user of your library that also uses yaml either directly or via some other library will get two copies of it in their application bundle, unless they too use the same re-packaging package.

Given that we're definitely moving towards an ES module world, my intent is to work towards maintaining just 'yaml' as the import path for the library. If some specifier is needed, that should be applied to the CommonJS endpoint.

Fundamentally, the reason you're getting a warning is probably the minimal CommonJS wrapper around the actual internal ES module implementation of the library's browser build, 'yaml/browser/dist/index.js'. And while that path may appear available, actually using it may start failing builds on Webpack 5, which obeys the package.json "exports" field.

The browser endpoints were in fact pure ES modules for a while, up until 1.9.3, but that needed to get reverted due to #163. Getting multi-platform releases to work right is hard, especially as I want the API to stay the same.

@eemeli
Copy link
Owner

eemeli commented Sep 23, 2020

Closing, as there's no clear action that ought to be taken here.

@eemeli eemeli closed this as completed Sep 23, 2020
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

No branches or pull requests

2 participants