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

@sentry/svelte is not a proper ESM package #10360

Closed
3 tasks done
GauBen opened this issue Jan 26, 2024 · 8 comments
Closed
3 tasks done

@sentry/svelte is not a proper ESM package #10360

GauBen opened this issue Jan 26, 2024 · 8 comments
Labels
Package: svelte Issues related to the Sentry Svelte SDK Stale Type: Bug

Comments

@GauBen
Copy link

GauBen commented Jan 26, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/svelte

SDK Version

7.98.0

Framework Version

Svelte 4

Link to Sentry event

No response

SDK Setup

None needed

Steps to Reproduce

https://stackblitz.com/edit/stackblitz-starters-h72nkr?file=index.js

  1. Use "type": "module" in package.json
  2. Import @sentry/svelte in Node.js

Expected Result

Should work

Actual Result

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/projects/stackblitz-starters-h72nkr/node_modules/svelte/src/runtime/ssr.js from /home/projects/stackblitz-starters-h72nkr/node_modules/@sentry/svelte/cjs/performance.js not supported.
Instead change the require of ssr.js in /home/projects/stackblitz-starters-h72nkr/node_modules/@sentry/svelte/cjs/performance.js to a dynamic import() which is available in all CommonJS modules.

The cause of this error is correctly described by https://publint.dev/@sentry/svelte@7.98.0

Tasks

Preview Give feedback
No tasks being tracked yet.
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 26, 2024
@github-actions github-actions bot added the Package: svelte Issues related to the Sentry Svelte SDK label Jan 26, 2024
@GauBen
Copy link
Author

GauBen commented Jan 26, 2024

I would open a merge request but I'm not familiar enough with your build system to do it without breaking anything.

Here's what I'd do:

  1. Output ESM files with .mjs extension instead of .js
  2. Add "type": "commonjs" to package.json
  3. Add "exports": ... to package.json with the following contents:
{
  "exports": {
    ".": {
      "types": "./types/index.d.ts",
      "import": "./esm/index.mjs",
      "require": "./cjs/index.js"
    }
  }
}

It also seems that many sentry packages have a broken package.json and/or ESM build:

I'd also add publint to the CI to ensure all packages are correctly build

@Lms24
Copy link
Member

Lms24 commented Jan 26, 2024

Hey @GauBen thanks for writing in!

Thanks for the input and I agree, our package config isn't ideal. This is on our radar for v8 as you've probably seen in #10046.

My question here is if you're currently experiencing problems within a Svelte App of yours? The reproduction link from your post doesn't contain a svelte app. As long as there's no actual problem with the current package structure, I'd avoid changing things in v7 just because of the publint warnings.

Note, We need to change the structure for Svelte 5 which we don't support yet. This is tracked in #10318 and will also happen at latest in v8 of our SDK (coming soon).

If you'd like to help out, feel free to open a PR - I'm happy to review. Also regarding publint, I'm not sure how this works in CI but I'm not opposed to adding it once we're actually ESM compatible (via #10046).

Please let me know if you think there's still an issue that we're not aware of. Otherwise I'm gonna close this issue to avoid too many open ones for the same problems.

@GauBen
Copy link
Author

GauBen commented Jan 26, 2024

Hey @Lms24, thanks for your quick reply!

I do have the very same error on a SvelteKit app with @sveltejs/adapter-node. The SSR build contains a chain of import that fails the exact same way as the plain Node.js reproduction above. We currently work around it using a patch that makes the package ESM only:

diff --git a/package.json b/package.json
index 9067b2baaf1b2c2923d4b8cf58c10aace01a3b3c..dc5d3386c85596ad2df2b31759ed1d57a5551880 100644
--- a/package.json
+++ b/package.json
@@ -6,6 +6,7 @@
   "homepage": "https://github.com/getsentry/sentry-javascript/tree/master/packages/svelte",
   "author": "Sentry",
   "license": "MIT",
+  "type": "module",
   "engines": {
     "node": ">=8"
   },
@@ -15,14 +16,10 @@
     "types",
     "types-ts3.8"
   ],
-  "main": "cjs/index.js",
-  "module": "esm/index.js",
-  "types": "types/index.d.ts",
-  "typesVersions": {
-    "<4.9": {
-      "types/index.d.ts": [
-        "types-ts3.8/index.d.ts"
-      ]
+  "exports": {
+    ".": {
+      "types": "./types/index.d.ts",
+      "import": "./esm/index.js"
     }
   },
   "publishConfig": {

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 26, 2024
@GauBen
Copy link
Author

GauBen commented Jan 26, 2024

@Lms24
Copy link
Member

Lms24 commented Jan 26, 2024

Hmm that's not great. Looked at your new reproduction and I still have a question: Are you actually declaring @sentry/svelte as a dependency in your project? Since you're using Sveltekit, I'd assume you're using our @sentry/sveltekit SDK, right? Generally, our SDK should be compatible with the Node adapter (in fact we do quite a lot to ensure it is).

Are you using any special Sentry setup or just the "standard" setup via the server and client hooks and vite config?

@GauBen
Copy link
Author

GauBen commented Jan 26, 2024

We do not actually declare any dependency as external, but not doing it in the repro will cause rollup to dismantle the package completely and only keep the few side effects. We use both @sentry/sveltekit and @sentry/svelte for historic reasons (our app predates the first release of @sentry/sveltekit). We are not doing anything fancy with our setup.

Is @sentry/sveltekit a drop in replacement for @sentry/svelte? If so I guess I should clean things up in my app

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 26, 2024
@Lms24
Copy link
Member

Lms24 commented Jan 26, 2024

There shouldn't be a reason to use @sentry/svelte in your sveltekit application. We export everything from @sentry/svelte in @sentry/sveltekit. So yes, it should basically be a drop in replacement. However, if you previously used @sentry/svelte I'd recommend going over the usages to check if they'er actually still necessary. For instance the SDK should only be initialized in the client and server hooks files. It's probably best to compare your setup to our Manual Setup guide in this case.

My suspicion is that declaring @sentry/svelte as a dependency makes Vite handle it differently than if it was just a transitive dependency of @sentry/sveltekit.

If this issue persists, please provide a reproduction with @sentry/sveltekit that includes how you initialize the SDK or how certain SDK exports/functions are used.

@getsantry
Copy link

getsantry bot commented Feb 19, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Feb 19, 2024
@getsantry getsantry bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: svelte Issues related to the Sentry Svelte SDK Stale Type: Bug
Projects
Archived in project
Development

No branches or pull requests

2 participants