Angular 10 CommonJS import warning

natholas commented 4 years ago

When building an app with Angular 10 it warns you about lottie-web.

Here is what it says on the angular blog:

When you use a dependency that is packaged with CommonJS, it can result in larger slower applications. Starting with version 10, we now warn you when your build pulls in one of these bundles. If you’ve started seeing these warnings for your dependencies, let your dependency know that you’d prefer an ECMAScript module (ESM) bundle.

sgClaudia98 commented 4 years ago

Use the Angular wrapper https://github.com/chenqingspring/ng-lottie

It helps you Reduce lottie-web bundle

Any way, to remove the warning on Angular Add this code to de angular.json

"build": {
  "builder": "@angular-devkit/build-angular:browser",
  "options": {
     "allowedCommonJsDependencies": [
        "lottie-web"
     ]
     ...
   }
   ...
},

Angular Docs

kawazoe commented 3 years ago

@sgClaudia98 Disabling warnings is not a fix. Plus, the link that you included to reduce the bundle size actually suggests to... not use lottie-web!

Every browser natively supports ECMAScript modules today and those type of modules help fix exactly what ngx-lottie is complaining about in the link you've posted. This is not a problem specific to angular either. It's a problem in vue, react, and any other project that wants to include lottie without renderers that are not used by the site.

There should be an ESM build of the library so that bundlers can tree-shake the code. Those modules are, quite literally, the standard now and it would make everyone's life much easier when trying to optimize their site.

From what I am seeing in the code, I understand that this would be a big effort. The library still uses patterns from 10 years ago to manage its modules and so a big refactor would probably be required to support this, but the advantages are equally as big, both for users and for maintainers.

bodymovin commented 2 years ago

@kawazoe Unfortunately, since lottie depends on json files, it can't know beforehand which modules to discard, and which ones it can keep. So I'm not sure tree shaking would be of much help here. That's one of the reasons why the build process is manual and it exposes lottie versions for most scenarios.

kawazoe commented 2 years ago

@bodymovin I understand that the effects/elements/etc can't be known at compile time with the current API. It could be possible to limit which ones can be loaded, similar to how CSS libraries like bootstrap let you remove components you don't use from the final bundle, but that would require more changes and would be a very involved optimization to use.

It's not really what I was expecting either. I was mostly interested in tree shaking the various renderers. In fact, with a very small change to how animations are loaded, all the build variants could be replaced with automatic tree shaking which would pretty much remove the need for the custom build script in ES module projects.

For instance, this code taken from the README:

lottie.loadAnimation({
  container: element, // the dom element
  renderer: 'svg',
  animationData: animationData, // the animation data
  rendererSettings: {
    ...
  }
});

Would become something like:

import { loadAnimation, createSvgRenderer } from 'lottie';

loadAnimation({
  container: element, // the dom element
  renderer: createSvgRenderer({
    ...  //< replaces rendererSettings
  }),
  animationData: animationData, // the animation data
});

bodymovin commented 2 years ago

So I've been working on migrating all code as modules and bundling them using rollup. Since I don't want to break compatibility with the previous version, and I also want to make a simple interface for users, I'm exposing all renderers as modules that can be imported. Here is the branch https://github.com/airbnb/lottie-web/tree/85_es6_modules And here are the modules that should be imported to select one of the lottie versions https://github.com/airbnb/lottie-web/tree/85_es6_modules/player/js/modules If you have some time and can check it out, I'd be grateful for any feedback.

kawazoe commented 2 years ago

I see where you are going with this. It is an interesting solution, but I believe it will cause more problem with how some people and modern tools expect your package to be published than you expect.

You will want to add a module key in your package.json to hint that you are compatible with es modules: https://rollupjs.org/guide/en/#publishing-es-modules . There is also more info over there: https://github.com/rollup/rollup/wiki/pkg.module . With that in mind, you can only specify one of your build configuration in there. This is because, again, es modules expects the user to pick which part of the library they want. They deal with the bundling; not you. That's why I changed the import syntax to specifically include createSvgRenderer in me previous post. It is expected that, because I do not use something, it is not included. Thus, with that syntax, because I did not use the canvas renderer, it will not get imported. In ESM land, people don't expect to have different builds for that.

Imagine a CJS user that used to import your full library. If your module key points to the full version, there is no problem there. The transition will be seamless because it matches the current CJS main key.

Now, imagine a user that previously used a specific build, they will have to update their import declaration and specifically import the ESM version. This is slightly cumbersome. The kind of module shouldn't be part of the code, specifically because you don't want to update your code every time the package changes how it deals with its bundles. Ideally, you want to leave this part to the build engine, but it can't decide because it only reads the package.json file which doesn't know about this more specific version. It is only aware of what is in the module key. The user could setup custom build rules if their tool supports them, like webpack aliases, but then it becomes even more convoluted because those are meant as a compatibility tool and often need to be manually synchronized with other tools likes the TypeScript's tsconfig path aliases.

This ends up more problematic if the user want to setup lottie with their own config. They cannot import from 'lottie-web' because this will impose the full configuration out of the box through hidden side-effects in the module. They have to import from 'lottie-web/player/js/modules/main' and that is not something obvious, specially since nothing will appear to be wrong if they do the former.

Even worse, if they use TypeScript, types won't load properly because they are associated with the root module (the one in the module or main key) and not sub modules like these. Any attempt at importing anything but the main ES Module will be incredibly difficult and most people will simply give up on them.

Thinking about this more, I'm not sure you can remain backward compatible with this change and provide an easy way for ESM people to import your library. Even if you kept your idea for the CJS modules, but exposed a whole new ESM bundle based on a barrel file that exports everything for the user to pick and choose from, you would still end up with two different configuration APIs. The TypeScript typings you provide will always be different between the two module format because of this and you cannot express that in your package.json...

I'd really like for someone with more experience with publishing big projects with a large audience to chip in. To me, this feels like the wrong direction. The only thing that I can say with confidence is that, as someone with experience dealing with ESM modules, I would not expect to have to change my import path and/or do a whole bunch of tooling config to not import part of a library. While it is backward compatible with your existing CJS module, it is not a "simple interface for users".

bodymovin commented 2 years ago

Agree, and I'm willing to break compatibility. But if I do, I'd definitely want a solution that will last. And since I'm not an expert on setting up these types of configurations, I'll try to reach out to get some help.
There's also a caveat regarding using the library from a script tag. Since this is an animation library and there's designers that use it that prefer not to deal with code, whatever I end up building, I have to make sure that I still offer a "one line" solution for those scenarios. Thanks for all the info, it's really helpful. I'll digest it some more, do some research and keep iterating. Fortunately, the heavy part of migrating everything to modules is done, so now it's a matter of using it correctly.