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(node-profiling): Output ESM and remove Sentry deps from output #11135

Merged
merged 7 commits into from Mar 15, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Mar 15, 2024

  • Moves Sentry deps to dependencies rather than devDependencies
  • Updated package.json exports
  • Updated the rollup config to mirror our other modules with the addition of the commonJs plugin
  • I couldn't use createRequire myself because Jest can't handle import.meta so instead I used @rollup/plugin-esm-shim which shims both require and __dirname in cpu_profiler.ts in the ESM output!

This comment was marked as outdated.

@timfish timfish marked this pull request as ready for review March 15, 2024 18:12
@AbhiPrasad AbhiPrasad merged commit 643eff2 into develop Mar 15, 2024
109 checks passed
@AbhiPrasad AbhiPrasad deleted the timfish/node-profiling-esm-and-bundle branch March 15, 2024 18:27
@AbhiPrasad
Copy link
Member

Little scared about edge cases with bundling, but lets see how it goes!

@JonasBa
Copy link
Member

JonasBa commented Mar 15, 2024

Eghhh, I dont think this will work, but lets see. The reason is that ESM doesnt know what to do with .node modules by default whereas CJS does, and it's unclear here if the require shim uses the CJS loader or not, if it does then this will work, else it'll break with unknown module/configure loader message.

Afaik what others do is they ship the equivalent of our cpu_profiler.ts or the one file in the source which actually imports the bindings as CJS, which means that 99% of the application is ESM, but then when you need to load the bindings, you force CJS so that the loader knows how to properly open it. Lets see how well this works, I'd be interested to see if it works

@timfish
Copy link
Collaborator Author

timfish commented Mar 15, 2024

We could always add another export @sentry/profiling-node/cjs which points at the cjs output and can be used if your bundler can't handle our ESM output?

Or alternatively output cpu_profiler.cjs so that file is call CommonJs.

AbhiPrasad added a commit that referenced this pull request Mar 25, 2024
ref #9956

This is another attempt at
#11033 now that
#11135 has merged in.
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…etsentry#11135)

- Moves Sentry deps to `dependencies` rather than `devDependencies`
- Updated `package.json` exports
- Updated the rollup config to mirror our other modules with the
addition of the commonJs plugin
- I couldn't use `createRequire` myself because Jest can't handle
`import.meta` so instead I used `@rollup/plugin-esm-shim` which shims
both `require` and `__dirname` in `cpu_profiler.ts` in the ESM output!
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants