Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
ref(profiling): Conditionally shim cjs globals#13267
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
c3511d6
ref(profiling) conditionally shim cjs globals
JonasBa 4498049
ref(profiling) remove logs and add separate build with shims
JonasBa 17c4122
ref(profiling) run test
JonasBa 8d801eb
ref(profiling) output mjs
JonasBa fc1a9f8
ref(profiling) add missing dirname import
JonasBa c5feb00
Merge branch 'develop' into jb/profiling/conditional-shim
JonasBa 773b298
Merge branch 'develop' into jb/profiling/conditional-shim
JonasBa cf53b90
ref: build and test as a single command
JonasBa f1f9bd4
build: revert changes to build.yml
JonasBa 1b22002
ref: correct build command
JonasBa d6adbfa
ref: add --experimental-require-module to test
JonasBa 7aecbb5
ref: bump to node 22 and run using the experimental flag
JonasBa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
29 changes: 29 additions & 0 deletions
29 dev-packages/e2e-tests/test-applications/node-profiling/build.shimmed.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| `, | ||
| }, | ||
| }); |
6 changes: 3 additions & 3 deletions
6 dev-packages/e2e-tests/test-applications/node-profiling/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
JonasBa marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // 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(); | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little hesitant about this because it means that CI runs differently than local - but I guess we can try this out, cc @mydea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is OK - this is just for the e2e tests, we do not test if this works in node 18 then, not sure how important this is to us.