fix(sveltekit): Add import attribute for node exports

[eltigerchino]

This PR changes the node export in the package.json file to include an import version (similar to the other exports) to fix sveltejs/kit#13869 . I'm not sure why the import key was left out but it seems to exist under the "module" key already, just that Vite never resolves to that one.

In SvelteKit, we're now bundling dependencies on the server that specify SvelteKit as a dependency or peerDependency. This has caused an issue where builds with @sentry/sveltekit were being bundled incorrectly. Adding the import attribute fixes this so that Vite resolves to the ESM build of Sentry.

• If you've added code that should be tested, please add tests.
  • Not sure if this needs a test. Let me know if I should add one.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).
  • Lint passes but I can't seem to get the ember tests to pass locally although I haven't changed anything there

[Lms24]

Hey @eltigerchino thanks for fixing this! I started CI to ensure this change doesn't break on older SvelteKit versions.

In SvelteKit, we're now bundling dependencies on the server that specify SvelteKit as a dependency or peerDependency.

hmm so the Sentry SDK is not getting bundled? This will cause issues once we find an OpenTelemetry-compatible way of initializing the Server-side SDK (import-in-the-middle cannot be bundled). I wonder if we'll need to work around this by removing the peer dependency on SvelteKit in some way. For now though, if this works, let's be pragmatic and merge this in.

(just assigning myself to the PR to indicate that I'm reviewing it)

[eltigerchino]

Hey @eltigerchino thanks for fixing this! I started CI to ensure this change doesn't break on older SvelteKit versions.

In SvelteKit, we're now bundling dependencies on the server that specify SvelteKit as a dependency or peerDependency.

hmm so the Sentry SDK is not getting bundled? This will cause issues once we find an OpenTelemetry-compatible way of initializing the Server-side SDK (import-in-the-middle cannot be bundled). I wonder if we'll need to work around this by removing the peer dependency on SvelteKit in some way.

The @sentry/sveltekit dep is bundled because of that peerDependency on @sveltejs/kit. Is it using any of kit’s exports? Meanwhile, @sentry/node and @sentry/core are not bundled because they have no peerDependency on kit. Are those what you mean by the SDK?

[Lms24]

Ah sorry, I misstyped -- ignore the "not" in my previous reply 🤦‍♂️ (as in, I'm concerned about the SDK getting bundled instead of not bundled). However, if it'll only bundle @sentry/sveltekit but stop at the border where we import @sentry/node we should be good 🤞

it using any of kit’s exports?

Yes:

• We import { navigating, page } from '$app/stores'; in our client side routing instrumentation (we need a way to listen to routing changes on the client, see here)
• we dynamically import kit to get the VERSION on the server-side (see here).
• various type exports (which should be fine?)

I didn't merge this PR yet btw because for some reason, our SvelteKit<>Cloudflare e2e test app is failing with this change. Currently investigating what could be causing this. I suspect some kind of collision with the worker export 🤔

(sorry that this is taking so long, this day was really busy on our end)

[Lms24]

@eltigerchino I re-bumped the kit versions in this PR, since we first pinned them to 2.21.2 in #16529 to unblock our repo and debugged the failing cloudflare test a bit. It seems like, since we're now bundling @sentry/sveltekit, we also pull in our build time exports (basically everything around the sentrySvelteKit vite plugin) into the server runtime code.

More specifically, I can now find a chunk called .svelte-kit/output/server/chunks/sourceMaps.js. This chunk contains parts of the server SDK but also this:

import "@sentry/vite-plugin";

which we never import in runtime code but only our vite plugin code:

sentry-javascript/packages/sveltekit/src/vite/sourceMaps.ts

Line 5 in f21815a

import { sentryVitePlugin } from '@sentry/vite-plugin';

any idea how we can keep this out of the newly bundled way? I'm kinda getting NextJS flashbacks here, where all of a sudden our webpack plugin also ended up in runtime code causing this issue. Back then, we had to work around this with a dynamic import of the plugin which caused a million of other issues. Really hoping we could avoid this here 😅

[eltigerchino]

@eltigerchino I re-bumped the kit versions in this PR, since we first pinned them to 2.21.2 in #16529 to unblock our repo and debugged the failing cloudflare test a bit.

Thank you!

It seems like, since we're now bundling @sentry/sveltekit, we also pull in our build time exports (basically everything around the sentrySvelteKit vite plugin) into the server runtime code.

More specifically, I can now find a chunk called .svelte-kit/output/server/chunks/sourceMaps.js. This chunk contains parts of the server SDK but also this:

import "@sentry/vite-plugin";

which we never import in runtime code but only our vite plugin code:

sentry-javascript/packages/sveltekit/src/vite/sourceMaps.ts

Line 5 in f21815a

import { sentryVitePlugin } from '@sentry/vite-plugin';

any idea how we can keep this out of the newly bundled way? I'm kinda getting NextJS flashbacks here, where all of a sudden our webpack plugin also ended up in runtime code causing this issue. Back then, we had to work around this with a dynamic import of the plugin which caused a million of other issues. Really hoping we could avoid this here 😅

I've added "sideEffects": "false" to the Kit package.json which is also present in the sentry core and node package.json files. I've also skimmed through the sentry/sveltekit code and there are indeed no side-effects so this should be safe. The dead code is now eliminated correctly during bundling.

[Lms24]

Ah nice, thanks for fixing the last test! CI seems to pass now. I just made a couple of cleanup changes but once everything is ready I'll merge it and see that we cut an SDK release today.

Thanks for fixing this -- really appreciate it!

[tehnrd]

I think this change may have introduced a memory leak. Mentioning issue #16674 here to keep it all link together.