amplitude / Amplitude-TypeScript

TypeScript Amplitude Analytics SDK
https://amplitude.github.io/Amplitude-TypeScript/
MIT License
132 stars 38 forks source link

Angular non-optimized CommonJS or AMD module #268

Open ChrisJohns-me opened 2 years ago

ChrisJohns-me commented 2 years ago

After installing @amplitude/analytics-browser v1.6.1 onto an Angular project, running ng serve causes Angular to throw a warning:

Warning: node_modules\@amplitude\analytics-browser\lib\esm\index.js depends on '@amplitude/analytics-types'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies
Warning: node_modules\@amplitude\analytics-browser\lib\esm\plugins\context.js depends on '@amplitude/ua-parser-js'. CommonJS or AMD dependencies can cause optimization bailouts.        
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

On Angular's website, they warn:

It is recommended that you avoid depending on CommonJS modules in your Angular applications. Depending on CommonJS modules can prevent bundlers and minifiers from optimizing your application, which results in larger bundle sizes. Instead, it is recommended that you use ECMAScript modules in your entire application. For more information, see How CommonJS is making your bundles larger.

This causes unoptimized and un-treeshakeable code, and causes our production code to be larger than intended (+100KB).

Expected Behavior

ng serve

Current Behavior

ng serve outputs a warning for CommonJS or AMD dependencies to not be used.

Possible Solution

Update the Amplitude codebase to use ECMAScript Modules.

Steps to Reproduce

  1. npm install @amplitude/analytics-browser
  2. Add an import to @amplitude/analytics-browser into your TypeScript codebase
  3. Run ng serve

Environment

kevinpagtakhan commented 2 years ago

Hi @ChrisJohns-me, thank you for choosing Amplitude. The packages in this repo are available in both CommonJS and ESM formats.

For @amplitude/analytics-types, it's package.json includes: https://github.com/amplitude/Amplitude-TypeScript/blob/f556fd2194aebce71de5b2b53c12e611d8c9b5a7/packages/analytics-types/package.json#L8-L10

My understanding is that with module pointing to an ESM file, bundlers use this during bundle time to perform tree shaking. Let me know what you think. We're happy to work with you on this issue.

For @amplitude/ua-parser-js, this is actually a fork of https://github.com/faisalman/ua-parser-js and bloats our SDK bundle size (biggest offender). Our roadmap includes work to remove this as dependency.

kevinpagtakhan commented 1 year ago

Hi @ChrisJohns-me, wanted to give you an update. Getting rid of our dependency on @amplitude/ua-parser-js is still very important to us and me personally. Once addressed, this should help with the warnings you have surfaced here. Thank you.

yuhao900914 commented 1 year ago

Created a ticket on our end to optimize the bundle size by removing or improving the @amplitude/ua-parser-js.

hanna-becker commented 1 year ago

+1 to this issue