From 7fa366ffc4ae849358747607d154eb36f67861ac Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 10 Sep 2024 05:05:07 -0400 Subject: [PATCH] ref(profiling): Conditionally shim cjs globals (#13267) The shims should only be applied if the globals are not present, else it results in double decl and a runtime error. The profiling SDK should gracefully handle env where the shims are already provided. I couldn't find a way to modify the shim as it is hardcoded in the plugin we are using so I went with the replace plugin approach and a placeholder value #poormansmacros. --- .github/workflows/build.yml | 2 +- .../node-profiling/build.shimmed.mjs | 29 +++++++++++++ .../node-profiling/package.json | 6 +-- packages/profiling-node/rollup.npm.config.mjs | 43 +++++++++++++++++-- packages/profiling-node/src/cpu_profiler.ts | 6 +++ 5 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/node-profiling/build.shimmed.mjs diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 626955ac2718..38102ff204c7 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1168,7 +1168,7 @@ jobs: - name: Set up Node uses: actions/setup-node@v4 with: - node-version-file: 'dev-packages/e2e-tests/package.json' + node-version: 22 - name: Restore caches uses: ./.github/actions/restore-cache with: diff --git a/dev-packages/e2e-tests/test-applications/node-profiling/build.shimmed.mjs b/dev-packages/e2e-tests/test-applications/node-profiling/build.shimmed.mjs new file mode 100644 index 000000000000..c45e30539bc0 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-profiling/build.shimmed.mjs @@ -0,0 +1,29 @@ +// Because bundlers can now predetermine a static set of binaries we need to ensure those binaries +// actually exists, else we risk a compile time error when bundling the package. This could happen +// if we added a new binary in cpu_profiler.ts, but forgot to prebuild binaries for it. Because CI +// only runs integration and unit tests, this change would be missed and could end up in a release. +// Therefor, once all binaries are precompiled in CI and tests pass, run esbuild with bundle:true +// which will copy all binaries to the outfile folder and throw if any of them are missing. +import esbuild from 'esbuild'; + +console.log('Running build using esbuild version', esbuild.version); + +esbuild.buildSync({ + platform: 'node', + entryPoints: ['./index.ts'], + outfile: './dist/index.shimmed.mjs', + target: 'esnext', + format: 'esm', + bundle: true, + loader: { '.node': 'copy' }, + banner: { + js: ` + import { dirname } from 'node:path'; + import { fileURLToPath } from 'node:url'; + import { createRequire } from 'node:module'; + const require = createRequire(import.meta.url); + const __filename = fileURLToPath(import.meta.url); + const __dirname = dirname(__filename); + `, + }, +}); diff --git a/dev-packages/e2e-tests/test-applications/node-profiling/package.json b/dev-packages/e2e-tests/test-applications/node-profiling/package.json index 94ec4926f2f6..a4c4bf1284fe 100644 --- a/dev-packages/e2e-tests/test-applications/node-profiling/package.json +++ b/dev-packages/e2e-tests/test-applications/node-profiling/package.json @@ -4,9 +4,9 @@ "private": true, "scripts": { "typecheck": "tsc --noEmit", - "build": "node build.mjs", - "test": "npm run build && node dist/index.js", - "clean": "npx rimraf node_modules", + "build": "node build.mjs && node build.shimmed.mjs", + "test": "node dist/index.js && node --experimental-require-module dist/index.js && node dist/index.shimmed.mjs", + "clean": "npx rimraf node_modules dist", "test:build": "npm run typecheck && npm run build", "test:assert": "npm run test" }, diff --git a/packages/profiling-node/rollup.npm.config.mjs b/packages/profiling-node/rollup.npm.config.mjs index 1cc4f0936954..12492b7c83e8 100644 --- a/packages/profiling-node/rollup.npm.config.mjs +++ b/packages/profiling-node/rollup.npm.config.mjs @@ -1,12 +1,49 @@ import commonjs from '@rollup/plugin-commonjs'; -import esmshim from '@rollup/plugin-esm-shim'; import { makeBaseNPMConfig, makeNPMConfigVariants } from '@sentry-internal/rollup-utils'; -export default makeNPMConfigVariants( +export const ESMShim = ` +import cjsUrl from 'node:url'; +import cjsPath from 'node:path'; +import cjsModule from 'node:module'; + +if(typeof __filename === 'undefined'){ + globalThis.__filename = cjsUrl.fileURLToPath(import.meta.url); +} + +if(typeof __dirname === 'undefined'){ + globalThis.__dirname = cjsPath.dirname(__filename); +} + +if(typeof require === 'undefined'){ + globalThis.require = cjsModule.createRequire(import.meta.url); +} +`; + +function makeESMShimPlugin(shim) { + return { + transform(code) { + const SHIM_REGEXP = /\/\/ #START_SENTRY_ESM_SHIM[\s\S]*?\/\/ #END_SENTRY_ESM_SHIM/; + return code.replace(SHIM_REGEXP, shim); + }, + }; +} + +const variants = makeNPMConfigVariants( makeBaseNPMConfig({ packageSpecificConfig: { output: { dir: 'lib', preserveModules: false }, - plugins: [commonjs(), esmshim()], + plugins: [commonjs()], }, }), ); + +for (const variant of variants) { + if (variant.output.format === 'esm') { + variant.plugins.push(makeESMShimPlugin(ESMShim)); + } else { + // Remove the ESM shim comment + variant.plugins.push(makeESMShimPlugin('')); + } +} + +export default variants; diff --git a/packages/profiling-node/src/cpu_profiler.ts b/packages/profiling-node/src/cpu_profiler.ts index 9ab470e2ca70..fb739a939e77 100644 --- a/packages/profiling-node/src/cpu_profiler.ts +++ b/packages/profiling-node/src/cpu_profiler.ts @@ -15,6 +15,12 @@ import type { } from './types'; import type { ProfileFormat } from './types'; +// #START_SENTRY_ESM_SHIM +// When building for ESM, we shim require to use createRequire and __dirname. +// We need to do this because .node extensions in esm are not supported. +// The comment below this line exists as a placeholder for where to insert the shim. +// #END_SENTRY_ESM_SHIM + const stdlib = familySync(); const platform = process.env['BUILD_PLATFORM'] || _platform(); const arch = process.env['BUILD_ARCH'] || _arch();