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

feat: add ESM builds for packages used in browser #2112

Merged
merged 4 commits into from
Apr 22, 2021

Conversation

t2t2
Copy link
Contributor

@t2t2 t2t2 commented Apr 15, 2021

Which problem is this PR solving?

Continuation of open-telemetry/opentelemetry-js-api#25:

While typescript uses ESM syntax, the code distributed on npm (build dir) has been transpiled to use commonjs require's for better compatibility. However this means that bundlers like webpack and rollup can't do tree shaking to remove code not imported by user, leaving behind unused dead code.

Fixes #1378
Fixes #1253

Short description of the changes

Added config files for tsc to build esm versions into build/esm of each package that is used in browser (as determined by existence of test:browser script in it's package.json, plus @opentelemetry/context-zone, @opentelemetry/propagator-b3 and @opentelemetry/resources) and referenced it in their package.json as module entrypoint. There was also a few errors that esm builds added that were fixed (using array spread syntax with a generator and importing lodash.merge)

Effect on bundle size

Test was done by doing npm pack on each package (+ current head version of @opentelemetry/api) and then placed in the right folder inside examples/tracer-web/node_modules/@opentelemetry. (I did try using npm links but that attempt didn't end well)
Devtool commented out in examples/tracer-web/webpack.config.js and built using npx webpack --mode production --profile --json > stats.json to generate stats file for better analysis.

Built asset With "module" removed from package.json
(forcing cjs usage)
With "module" in package.json
fetch.js 181617 164222
metrics.js 91895 80365
xml-http-request.js 183205 165730
zipkin.js 86583 68703

image

Comparison using webpack visualizer. Left - cjs, Right - ESM (note how individual files aren't shown)

Potential improvements

  • lodash.merge can't be normally imported as it does module.exports = func so it uses cjs require(). lodash merge #1928 already highlighted it's size being an issue so this is another reason to replace it
  • While update-ts-references added support for custom tsconfig filenames in v2.3.0, the generated references don't include that custom filename, so /tsconfig.esm.json needs to be manually updated.

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #2112 (6ab6fbf) into main (7775c0e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2112   +/-   ##
=======================================
  Coverage   92.76%   92.76%           
=======================================
  Files         140      140           
  Lines        4987     4987           
  Branches     1029     1029           
=======================================
  Hits         4626     4626           
  Misses        361      361           
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/Meter.ts 88.59% <100.00%> (ø)
...ackages/opentelemetry-metrics/src/MeterProvider.ts 88.57% <100.00%> (ø)
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 96.61% <100.00%> (ø)

packages/opentelemetry-web/tsconfig.esm.json Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to be able to automate the generation of this file so that the new entry will be added or removed automatically. I'm fine with doing this either in this PR or as a separate PR. Some js script that will find all tsconfig.esm.json files in packages folder then will update references and create a new file.
WDYT ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the references be in the base tsconfig being extended?

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
@@ -0,0 +1,11 @@
{
Copy link
Member

@dyladan dyladan Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no references?

edit: thinking more it might not be needed for individual package esm builds to have the references since it's only used in local dev.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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

Successfully merging this pull request may close these issues.

ship esm version Support ECMA support due Angular 10 shows warnings
5 participants