First class ESM support #113
Conversation
There was a problem hiding this comment.
Required for Rollup not to hang at the end of the build for no apparent reason, probably due to an underlying bug in the TypeScript compiler but I don't want to open that can of worms. Created a PR on the Rollup repo to add this as a CLI flag rollup/rollup#5195
| await app.listen({ port, host: "0.0.0.0" }) | ||
| await app.pg.query(schema) |
There was a problem hiding this comment.
ESM support gets us top level await
examples/eslint.config.js
Outdated
There was a problem hiding this comment.
ESLint configs in directories with a package.json with "type": "module" all have to be converted to ESM syntax
| try { | ||
| module.exports.oboe = require(`@solarwinds-apm/bindings-${t}/oboe.node`) | ||
| } catch (cause) { | ||
| module.exports.oboe = new Error(`unsupported platform ${t}`, { cause }) | ||
| } | ||
|
|
||
| const e = (module.exports = {}) | ||
| nativeRequireAssign("oboe", e) | ||
| nativeRequireAssign("metrics", e) | ||
| try { | ||
| module.exports.metrics = require(`@solarwinds-apm/bindings-${t}/metrics.node`) | ||
| } catch (cause) { | ||
| module.exports.metrics = new Error(`unsupported platform ${t}`, { cause }) | ||
| } |
There was a problem hiding this comment.
Laying things out this way helps Node detect the exports during static analysis for ESM/CJS interop
| @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| export namespace eventLoop { | |||
| export declare namespace eventLoop { | |||
There was a problem hiding this comment.
Wrote those by hands and made a mistake, apparently this is the proper syntax.
| "files": [ | ||
| "dist/" | ||
| "./src/", | ||
| "./dist/" | ||
| ], |
There was a problem hiding this comment.
Including src in the published artifacts and generating sourcemaps in dist makes debugging much nicer. Previously we generated the sourcemaps but didn't bundle the source so they pointed to files that didn't exist.
| import { collectNodeModulesDependencies } from "./node-modules.js" | ||
| import { collectPnpApiDependencies, type PnpApi } from "./pnp-api.js" |
There was a problem hiding this comment.
ESM requires full file name and extensions in import. We use .js and not .ts cause TypeScript (by design) never touches file extensions in imports.
| // ts declarations | ||
| { | ||
| files: ["**/*.d.ts"], |
There was a problem hiding this comment.
This was only required because I was missing the declare keywords in manually written .d.ts files.
| import { | ||
| awsBeanstalkDetector, | ||
| awsEc2Detector, | ||
| awsEcsDetector, | ||
| awsEksDetector, | ||
| awsLambdaDetector, | ||
| } from "@opentelemetry/resource-detector-aws" | ||
| import { containerDetector } from "@opentelemetry/resource-detector-container" | ||
| import { gcpDetector } from "@opentelemetry/resource-detector-gcp" |
There was a problem hiding this comment.
Some of these detectors were spamming warnings, better just add them back if someone ever requests it.
| const TOKEN = "secret" | ||
| import base from "@solarwinds-apm/eslint-config" | ||
|
|
||
| module.exports = { | ||
| TOKEN, | ||
| } |
There was a problem hiding this comment.
This wasn't used in the example anymore and I guess git decided I actually moved the file and changed everything in it.
| "@opentelemetry/instrumentation-bunyan": "^0.32.1", | ||
| "@opentelemetry/instrumentation-fs": "^0.8.1", | ||
| "@opentelemetry/instrumentation-http": "~0.43.0", | ||
| "@opentelemetry/instrumentation-mysql2": "^0.34.1", | ||
| "@opentelemetry/instrumentation-pg": "^0.36.1", | ||
| "@opentelemetry/instrumentation-pino": "^0.34.1", | ||
| "@opentelemetry/instrumentation-winston": "^0.32.1", |
There was a problem hiding this comment.
This was to be able to import types but now we already have access to those types from the instrumentations package's dependencies.
packages/sdk/src/diag-logger.ts
Outdated
There was a problem hiding this comment.
I created this cause OTel's DiagConsoleLogger doesn't show levels or timestamps and it was making my life very painful.
packages/sdk/src/patches/lambda.ts
Outdated
There was a problem hiding this comment.
Without this the lambda instrumentation always prints a warning when used outside lambda.
| "engines": { | ||
| "node": ">=16" | ||
| "node": ">=16.13.0" | ||
| }, |
There was a problem hiding this comment.
Node 16.13 is the first Node 16 LTS and it properly supports ESM. Every previous 16.x release has been unsupported for years.
| if (setRegister && semver.gte(process.versions.node, "20.8.0")) { | ||
| setRegister() | ||
| module.register("./hooks.es.js", import.meta.url) | ||
| } |
There was a problem hiding this comment.
The --loader flag is deprecated in Node >=20.8 in favour of this new function.
| export async function init() { | ||
| const id = `${packageJson.name}@${packageJson.version}` | ||
| const initSymbol = Symbol.for(`${id}/init`) | ||
|
|
||
| if (!(initSymbol in globalThis)) { | ||
| Object.defineProperty(globalThis, initSymbol, { | ||
| value: true, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: false, | ||
| }) | ||
| // init only once | ||
| const setInit = setter("init") | ||
| if (!setInit) return | ||
| setInit() |
There was a problem hiding this comment.
Big diff here but it's just an early return and everything being moved out of the if statement and losing an indentation level.
|
|
||
| Packages should optimally target both ESM and CJS unless they have a good reason not to, in which case they should target CJS only for compatibility. At the moment the two CJS-only packages are the SDK because it's internal and has a lot of stateful components, and the bindings because they are internal and `.node` files can't be imported from ESM. | ||
|
|
||
| Dual targeting is made simple by the project's [Rollup configuration](./packages/rollup-config/), which should work without being extended pretty much all the time. Files ending in `.es.{ts, js}` will only be included for ESM `.cjs.{ts, js}` only for CJS. We don't use `.m{ts, js}` and `.c{ts, js}` because they have poor TypeScript support (and I, the original author, think they are Ugly and Bad). Projects should always specify `"type": "module"` when dual targeting (or an explicit `"commonjs"`) in their `package.json`. |
Use Rollup to target both ESM and CJS from most packages, using conditional exports to specify which entrypoint to use depending on the format.
Exports OTel's somewhat hidden
hooks.mjswhich themselves exportimport-in-the-middlehooks and provides both support for Node <20 with--loaderflag and >=20 with--importflag. The old-rflag will still work for CJS but doesn't support ESM at all, which the others support both ESM and CJS.Most of the examples have been migrated to ESM, I'm planning to do the same with scripts eventually.