Add ESM build

[t2t2]

Why

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.

While @opentelemetry/api has lesser tree shake potential due to code with side effects, I figured the best way to tackle this is to first target the one package in it's own repo, and once good implementation is figured out copy it to packages in open-telemetry/opentelemetry-js and open-telemetry/opentelemetry-js-contrib.

Relevant:
open-telemetry/opentelemetry-js#1378
open-telemetry/opentelemetry-js#1253

How about using type: module and exports? (as shown off by this tweet)

It's plausible but it may requires changes that might not be the best to do right before 1.0.0 release. Open this section for my attempt at it

// package.json
{
  "type": "module",
  "exports": {
    ".": "./build/lib/index.js",
    "./lib/platform/index.js": {
      "browser": "./build/lib/platform/browser/index.js",
      "default": "./build/lib/platform/node/index.js"
    },
    "./lib": "./build/lib/*.js"
  },
}

While this will work instantly with rollup, there were issues with webpack and node.js. And well, second likely explains first. When package.json has type: module, node v12+ will also expect to see ESM code in *.js files and will use the file pointed exports map. There is however a difference - node.js requires file extensions when importing from files:

node:internal/process/esm_loader:74
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'repos/opentelemetry-js-api/lib/baggage/internal/baggage' imported from repos/opentelemetry-js-api/lib/baggage/index.js

Similar error was also given by webpack, so it's probably just them copying nodejs behaviour

This can be fixed by adding .js to the end of imports in .ts files which seems odd as js files don't exist until built, but is the recommended way

There's also considerations needed for possible duplicated code loading when used by both esm and commonjs consumers and possible backwards compatibility breakage for imports from direct files (import ... from '@opentelemetry/api/build/src/....), so would rather not try to do all of this right now.

How well does it work?

As noted above, due to side effects in index.ts it won't throw everything unneeded away but doing this over-simplified and totally unrealistic example:

import {INVALID_SPANID} from '@opentelemetry/api';

console.log(INVALID_SPANID);

• Rollup configured with nodeResolve, commonjs and typescript plugins
• webpack with mode: development and optimization.concatenateModules = true(does tree shaking, is enabled with mode: production but I wanted to check human-readable output for signs of shaking)

# Using currently published @opentelemetry/api:

 82671 23 Mar 16:33 build/pre-esm/rollup/index.js
106104 23 Mar 16:31 build/pre-esm/webpack/index.js

after doing:
repos/opentelemetry-js-api % npm link
repos/test-bundling % npm link @opentelemetry/api

 48425 24 Mar 11:57 build/rollup/index.js
 69957 24 Mar 12:02 build/webpack/index.js

Potential improvements

• ESM versions could be used to ship code targeting newer targets (since esm support starts from node.js 12, es2019 can be considered), allowing users to decide if they want to transpile for older browsers or only for latest versions, which could have smaller output. But this can be considered backwards compatibility breaking for those who don't transpile dependencies (as it used to be common to ignore node_modules for babel-loader)
• Maybe folder naming? I've usually seen (root)/lib used for files generated from (root)/src but keeping the current structure I left build/src/ as-is and put esm version into build/lib/
• Rollup seems to ignore file overrides in package.browser was missing browser option in @rollup/plugin-node-resolve, works as expected with that

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Taavo-Taur "t2t2" Tammur (28b9a9a)

[codecov]

Codecov Report

Merging #25 (bd0aaaa) into main (504041a) will not change coverage.
The diff coverage is n/a.

❗ Current head bd0aaaa differs from pull request most recent head 0f8729d. Consider uploading reports for the commit 0f8729d to get more accurate results

@@           Coverage Diff           @@
##             main      #25   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files          39       39           
  Lines         504      504           
  Branches       81       81           
=======================================
  Hits          477      477           
  Misses         27       27           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 504041a...0f8729d. Read the comment docs.

[johnbley]

LGTM, good idea!

[dyladan]

Rollup seems to ignore file overrides in package.browser as I still found this in output:
var _globalThis = typeof globalThis === 'object' ? globalThis : global;

This could be quite a big problem. If the browser implementations are not used then there will be quite some things broken I would expect

[witemple-msft]

Having this should be very helpful for us in the Azure SDKs, as we use rollup to generate bundles for automated browser testing, and opentelemetry has historically been difficult to bundle (you can see an issue in rollup that's keeping us from migrating to newer bundles here: rollup/plugins#743, and opentelemetry triggers that bug). ESMs in the package should make bundling this package much more straightforward.

@t2t2 please ping me if there's anything I can do to help test this or debug any of the potential improvements you've listed above.

@richardpark-msft, @xirzec : JYFI

[dyladan]

@t2t2 Looks like the browser issue is fixed in a plugin rollup/rollup-plugin-node-resolve#8 Any way you can test real quick?

[t2t2]

@dyladan Yep did a bit more testing and turns out it was because I was missing browser option in the node-resolve plugin, all good with that

[mhennoch]

@dyladan @obecny @Flarna @javascript-approvers Can we get some more reviews on this?

[xirzec]

It would be so lovely to get this in before the next release, I wish I had thought of it sooner!

[dyladan]

I'm concerned that this seems to completely duplicate the generated output, doubling the extracted size almost exactly.

Current state
npm notice === Tarball Details === 
npm notice name:          @opentelemetry/api                      
npm notice version:       1.0.0-rc.0                              
npm notice filename:      opentelemetry-api-1.0.0-rc.0.tgz        
npm notice package size:  42.9 kB                                 
npm notice unpacked size: 183.9 kB                                
npm notice shasum:        0c7c3f5e1285f99cedb563d74ad1adb9822b5144
npm notice integrity:     sha512-iXKByCMfrlO5S[...]jjnp0+UHBwnDQ==
npm notice total files:   160                                     
npm notice 
opentelemetry-api-1.0.0-rc.0.tgz

With change applied
npm notice === Tarball Details === 
npm notice name:          @opentelemetry/api                      
npm notice version:       1.0.0-rc.0                              
npm notice filename:      opentelemetry-api-1.0.0-rc.0.tgz        
npm notice package size:  51.9 kB                                 
npm notice unpacked size: 336.0 kB                                
npm notice shasum:        e8ade70da33af934af6ee7050a918000db44e044
npm notice integrity:     sha512-fKdJorwrya4LV[...]Ae8jukQtbLXaA==
npm notice total files:   316                                     
npm notice 
opentelemetry-api-1.0.0-rc.0.tgz

This seems like an awfully high cost. Is there no way to generate files/output that is compatible with both commonJS and ESM?

[xirzec]

This seems like an awfully high cost. Is there no way to generate files/output that is compatible with both commonJS and ESM?

The main advantage, as I understand it, is that ESM customers can bundle/tree-shake what they need, rather than needing to dynamically import your package when distributing their app.

And I don't believe there is any way to bridge the gulf between commonjs and ESM, since they have completely different syntax that can't be dynamically detected (i.e. import is not a global function like require, but actually extends the grammar of the language.)

[witemple-msft]

IMHO, for only an additional ~160kB on disk (9kB on the wire), the benefits are very significant for browser consumers. This will be much friendlier for ESM-native bundlers that can decide to tree-shake the resulting code, hopefully resulting in smaller bundles and way simpler configuration at the expense of a slightly bigger package file.

Ordinary CJS is usually not so bad for bundlers, but the transforms that TypeScript emits to support its syntactic features in ES5+CJS environments are difficult for bundlers like rollup to analyze statically. For example, if you look at the compiled output of this package, TypeScript emits an __exportStar helper and calls it several times. It's useful for transpiling TypeScript to CJS, but unfortunately makes the exports of the package impossible to statically analyze in a generic way. If you refrain from using export * syntax and instead enumerate all the exports in index.ts, it would probably be better, but the ESM will always be preferred for bundling. We end up adding a lot of specialized configuration for @opentelemetry/api into our rollup configs and it has been a recurrent source of problems.

For what it's worth, for our Azure packages we ship both a CJS bundle (just a bundle of our code, no dependencies or anything other than our source) as the package "main" field as well as the ESM tree in the package "module" field. This seems like it offers a good balance of flexibility between bundling and using our packages with Node.

[dyladan]

Can we rename the package from build/lib to build/esm to make it clear why it's there?

[t2t2]

@dyladan Makes sense, renamed it