fix(sveltekit): Fix SvelteKit Cloudflare usage #14672

SG60 · 2024-12-11T20:26:12Z

Should close #13976. This adds a different set of exports for workers.

To use this, you have to use the new initCloudflareSentryHandle SvelteKit handler function, before your call to sentryHandle().

It should look something like this:

export const handle = sequence(
  initCloudflareSentryHandle({
    // YOUR SENTRY CONFIG HERE
  }),
  sentryHandle(),
  anyOtherCustomHandlers
);

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Ideally this would use OTEL, but that requires integrating another library, as the first-party OTEL libaries do not support Cloudflare workers.

…sdk in sveltekit Also reexports the Cloudflare SDK.

Lms24

Hey @SG60 thanks for opening this PR! Have you tested this already in a cloudflare deployment? While I think this might generally work, we need to ensure it actually does. I will also try to test this myself.

One thing I'm not too happy about is the amount of code duplication for the not CF-specific server APIs (sentryHandle, wrapLoad.., etc). Have you tried to re-export the respective APIs from the original files? Perhaps we need to move them to a common file that both, node and CF index files can re-export from?

SG60 · 2024-12-12T10:57:07Z

@Lms24 Thanks for having a look. I've tested with Wrangler v3 locally (running pnpm run build && pnpm wrangler pages dev .svelte-kit/cloudflare), but not in a remote Cloudflare deployment.

And yes, I agree that the code duplication isn't great! I'm sure a load of stuff could be put in a common file.

I did try just re-exporting from the original ./server files, but that pulls in too much of the Node SDK, and then SvelteKit refuses to build successfully. I think the issue is actually caused by the OpenTelemetry SDK being used by the Sentry Node SDK.

Handle functions and utils.ts There is more left to refactor as well

SG60 · 2024-12-12T12:28:49Z

@Lms24 How does that look now? I've moved a load of stuff into a new server-common directory.

Lms24

Thanks, this looks already a lot better :)

I didn't get to test this today unfortunately but I'll hopefully find some time on Monday.

I will also tag @AbhiPrasad, our resident CloudFlare expert, - does this look reasonable to you?

packages/sveltekit/src/worker/handle.ts

packages/sveltekit/src/server/handle.ts

SG60 · 2024-12-13T19:51:21Z

Great - thanks. I've replied to both review comments.

Lms24

So I gave this a try locally, by building the package and using yalc to install the local package. Unfortunately, I think something with the exports isn't yet working correctly. After making a modification to exports, I got a bit further and was able to at least get to the end of the sveltekit build (npm run build).

However, after the initial build, I think before the build starts the app for SSG, I get this error:

node:internal/event_target:1100
  process.nextTick(() => { throw err; });
                           ^
file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/.svelte-kit/output/server/chunks/hooks.server.js:9
import { continueTrace, initCloudflareSentryHandle } from "@sentry/node";
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@sentry/node' does not provide an export named 'initCloudflareSentryHandle'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:214:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async get_hooks (file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/.svelte-kit/output/server/chunks/internal.js:640:49)
    at async file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/.svelte-kit/output/server/index.js:2888:24
    at async Server.init (file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/.svelte-kit/output/server/index.js:2886:5)
    at async prerender (file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/node_modules/@sveltejs/kit/src/core/postbuild/prerender.js:454:2)
    at async MessagePort.<anonymous> (file:///Users/lukas/code/test-projects/sveltekit-cloudflare-test/node_modules/@sveltejs/kit/src/utils/fork.js:23:16)
Emitted 'error' event on Worker instance at:
    at [kOnErrorMessage] (node:internal/worker:326:10)
    at [kOnMessage] (node:internal/worker:337:37)
    at MessagePort.<anonymous> (node:internal/worker:232:57)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:826:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28)

Looking at the erroneous line, it seems that the output writes a wrong import statement into: .svelte-kit/output/server/chunks/hooks.server.js:

import { continueTrace, initCloudflareSentryHandle } from "@sentry/node";

Which makes me think that the actual SvelteKit build takes the "node" entry point while only the cloudflare adapter build afterwards uses "worker".

Does this correlate with your testing? I'd be curious to learn how you got this working locally otherwise :)

For reference, I pushed my test project to my GH if you wanna take a look as to what I need to change, in case I'm missing something: https://github.com/Lms24/sveltekit-cloudflare-test

Lms24 · 2024-12-16T12:47:39Z

packages/sveltekit/package.json

+      "worker": {
+        "import": "./build/esm/index.worker.js",
+        "require": "./build/cjs/index.worker.js"
+      },


not directly applicable to this line but: I had to make this change to the node export to start the build:

Suggested change

},

"node": {

"require": "./build/cjs/index.server.js",

"import": "./build/esm/index.server.js"

}

However, even after this change, I get another error a bit later

Sorry @Lms24 if this isn't the right place to discuss this, but is there any reason why we don't make this change in a separate PR?

I'm having an issue where trying to import import * as Sentry from '@sentry/sveltekit'; using ESM in Node.JS has all the @sentry/node imports under Sentry.default, and the above change seems to fix it.

From reading #9872 (comment), it looks like ESM used to be broken, but I think 7f2e804 might have fixed it? But a similar change for remix seems to have not worked: #12742

@aloisklink sorry for only seeing your comment now: I agree, we should make this change separately. Thing is, as far as I know, SvelteKit transpiles to CJS (at least for Node-based environments). So I'm a bit hesitant to add an ESM entry point, especially because this works even less well than it currently works in CJS, with OpenTelemetry.

Feel free to submit a PR and let's see what our tests have to see

Lms24 · 2024-12-16T13:00:10Z

In case you have a test app where this already works, you can also add an e2e test application (similarly to the sveltekit ones we already have but with the cloudflare adapter). Not strictly necessary, but it would give us a common testing ground.

Lms24 · 2024-12-16T13:45:17Z

For transparency: I also left a reply in the svelteKit issue after trying to swap out everything with @sentry/cloudflare directly in hooks.server.ts instead of using the changes proposed in this PR. I think we're still fundamentally limited by the build step in the cloudflare adapter but this might just get solved via sveltejs/kit#13132.

Again, if you get things working locally and I'm just missing something feel free to correct me :)

Lms24 · 2024-12-16T14:08:41Z

heads-up: I just assigned myself to this PR for internal tracking purposes since I'm reviewing it. Not gonna take it over :)

SG60 · 2024-12-17T10:00:15Z

Sorry, wasn't able to look at this yesterday, but I should be able to have a look later today. I have it working locally, so I just need to work out what I've got setup differently.

…it on cloudflare pages Seems to have an issue with wrangler hanging during the playwright test though.

SG60 · 2024-12-17T12:51:37Z

This e2e test app I've added runs fine locally for me.

I'm not sure why the Playwright tests aren't working for me locally though, it seems as though they time out - I think something to do with the way wrangler's built in server works.

Just test whether it builds successfully

SG60 · 2024-12-17T15:36:51Z

Ok, I've added a simpler e2e test app now, that just checks that the site builds and runs successfully with wrangler. It should all work.

I think the issue with your test app might just be the vite and vite-plugin-svelte versions. I'm testing with versions >6 and >5 respectively.

Lms24 · 2024-12-17T16:11:11Z

I think the issue with your test app might just be the vite and vite-plugin-svelte versions. I'm testing with versions >6 and >5 respectively.

Ahh that might play a role indeed. IIRC there was some change in Vite regarding which exports condition would be considered by default. Which would play a role here.

I'll take another look tomorrow. Thanks for the update and for adding the apps!

Lms24 · 2024-12-23T15:12:42Z

@SG60 sorry for only getting back to you now. I've upgraded vite to v6 and vite-plugin-svelte to v5 in my test app. However, I still get errors. One thing that seems to make a difference is package managers. I get further, specifically to the 2nd Cloudflare build step, with pnpm. With npm it already fails on the first build.

I do see though that the e2e test app builds correctly. I'm still trying to figure out what't different for the two apps, other than that I have to use a local version of the package in my test app (tried with linking, yalc and directly using the tarball).

One more thing: We unified continueTrace so that it now dynamically decides which version (code or OpenTelemetry) to use in #14813. This means we probably don't need to pass in the respective continueTrace implementation but we can always use the one from @sentry/core.

Lms24 · 2024-12-23T15:30:22Z

I found it! As soon as you add an export const prerender = true to any page, this build error is thrown:

node:internal/event_target:1100
  process.nextTick(() => { throw err; });
                           ^
file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/.svelte-kit/output/server/chunks/hooks.server.js:1
import { handleErrorWithSentry, sentryHandle, initCloudflareSentryHandle } from "@sentry/sveltekit";
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: Named export 'initCloudflareSentryHandle' not found. The requested module '@sentry/sveltekit' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@sentry/sveltekit';
const { handleErrorWithSentry, sentryHandle, initCloudflareSentryHandle } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:214:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async get_hooks (file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/.svelte-kit/output/server/chunks/internal.js:1484:49)
    at async file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/.svelte-kit/output/server/index.js:2981:24
    at async Server.init (file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/.svelte-kit/output/server/index.js:2979:5)
    at async prerender (file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/node_modules/.pnpm/@[email protected]_@[email protected][email protected][email protected]_@types+node@_za3kh67umycjwox5765ulc3c44/node_modules/@sveltejs/kit/src/core/postbuild/prerender.js:473:2)
    at async MessagePort.<anonymous> (file:///Users/lukas/code/sentry-javascript/dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/node_modules/.pnpm/@[email protected]_@[email protected][email protected][email protected]_@types+node@_za3kh67umycjwox5765ulc3c44/node_modules/@sveltejs/kit/src/utils/fork.js:23:16)
Emitted 'error' event on Worker instance at:
    at [kOnErrorMessage] (node:internal/worker:326:10)
    at [kOnMessage] (node:internal/worker:337:37)
    at MessagePort.<anonymous> (node:internal/worker:232:57)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:826:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28)

Which still suggests to me that something is going wrong with entry point resolution.

Lms24

Ok, so I think I found the most important things we still need to change.

Looks like the prerendering phase takes the node export from package.json and this fails because we don't export initCloudflareSentryHandle from index.server.ts. We can fix this by just exporting a simple stub for this from the server that just resolves the event.

So, for this PR to be merged:

As stated above, let's try to use the newly unified continueTrace from @sentry/core
Add the cloudflare init handle stub on the server entry point and adjust the index.types.ts to use the cloudflare version for types
Add another page to the e2e test app that's prerendered, so that we can cover both, prerendered and ssr'd pages.

Thanks for sticking with us!

Lms24 · 2024-12-23T15:51:31Z

dev-packages/e2e-tests/test-applications/sveltekit-cloudflare-pages/e2e/demo.test.ts

nit pick but can we rename the directory to /tests/demo.test.ts? Just to be consistent with our other e2e test apps.

SG60 added 2 commits December 11, 2024 20:14

fix(sveltekit): Don't use node apis in cloudflare workers

fc47659

Ideally this would use OTEL, but that requires integrating another library, as the first-party OTEL libaries do not support Cloudflare workers.

fix(sveltekit): provide a handle function to init cloudflare workers …

31aa1d4

…sdk in sveltekit Also reexports the Cloudflare SDK.

Lms24 reviewed Dec 12, 2024

View reviewed changes

SG60 added 4 commits December 12, 2024 11:41

chore(sveltekit): refactor some common server-side code

dc969b9

Handle functions and utils.ts There is more left to refactor as well

chore(sveltekit): refactor the rest of the common server-side code

218c2cf

fix(sveltekit): avoid importing fs and path in workers contexts

5aa9d54

Merge branch 'develop' into patch-1

28b2fd3

SG60 requested a review from Lms24 December 12, 2024 14:15

Lms24 reviewed Dec 13, 2024

View reviewed changes

packages/sveltekit/src/worker/handle.ts Show resolved Hide resolved

packages/sveltekit/src/server/handle.ts Show resolved Hide resolved

Lms24 mentioned this pull request Dec 16, 2024

Build Error in SvelteKit with Cloudflare Pages Following Cloudflare SvelteKit Framework Guide #13976

Open

3 tasks

Lms24 reviewed Dec 16, 2024

View reviewed changes

Lms24 self-assigned this Dec 16, 2024

test(sveltekit-cloudflare-pages): add an e2e test for @sentry/sveltek…

dc510d2

…it on cloudflare pages Seems to have an issue with wrangler hanging during the playwright test though.

test(sveltekit-cloudflare): simpler e2e test

2c443a8

Just test whether it builds successfully

Lms24 reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sveltekit): Fix SvelteKit Cloudflare usage #14672

fix(sveltekit): Fix SvelteKit Cloudflare usage #14672

SG60 commented Dec 11, 2024

Lms24 left a comment

SG60 commented Dec 12, 2024

SG60 commented Dec 12, 2024

Lms24 left a comment

SG60 commented Dec 13, 2024

Lms24 left a comment

Lms24 Dec 16, 2024

aloisklink Dec 17, 2024 •

edited

Loading

Lms24 Dec 23, 2024

Lms24 commented Dec 16, 2024

Lms24 commented Dec 16, 2024

Lms24 commented Dec 16, 2024

SG60 commented Dec 17, 2024

SG60 commented Dec 17, 2024 •

edited

Loading

SG60 commented Dec 17, 2024

Lms24 commented Dec 17, 2024

Lms24 commented Dec 23, 2024

Lms24 commented Dec 23, 2024 •

edited

Loading

Lms24 left a comment •

edited

Loading

Lms24 Dec 23, 2024

-      },
+  "node": {
+    "require": "./build/cjs/index.server.js",
+    "import": "./build/esm/index.server.js"
+  }

fix(sveltekit): Fix SvelteKit Cloudflare usage #14672

Are you sure you want to change the base?

fix(sveltekit): Fix SvelteKit Cloudflare usage #14672

Conversation

SG60 commented Dec 11, 2024

Lms24 left a comment

Choose a reason for hiding this comment

SG60 commented Dec 12, 2024

SG60 commented Dec 12, 2024

Lms24 left a comment

Choose a reason for hiding this comment

SG60 commented Dec 13, 2024

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Dec 16, 2024

Choose a reason for hiding this comment

aloisklink Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Lms24 Dec 23, 2024

Choose a reason for hiding this comment

Lms24 commented Dec 16, 2024

Lms24 commented Dec 16, 2024

Lms24 commented Dec 16, 2024

SG60 commented Dec 17, 2024

SG60 commented Dec 17, 2024 • edited Loading

SG60 commented Dec 17, 2024

Lms24 commented Dec 17, 2024

Lms24 commented Dec 23, 2024

Lms24 commented Dec 23, 2024 • edited Loading

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

Lms24 Dec 23, 2024

Choose a reason for hiding this comment

aloisklink Dec 17, 2024 •

edited

Loading

SG60 commented Dec 17, 2024 •

edited

Loading

Lms24 commented Dec 23, 2024 •

edited

Loading

Lms24 left a comment •

edited

Loading