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

Cloudflare adapter worker build is resolving client-side files from libraries #12813

Open
Lms24 opened this issue Oct 15, 2024 · 12 comments · Fixed by #13132 · May be fixed by #13427 or #13072
Open

Cloudflare adapter worker build is resolving client-side files from libraries #12813

Lms24 opened this issue Oct 15, 2024 · 12 comments · Fixed by #13132 · May be fixed by #13427 or #13072

Comments

@Lms24
Copy link
Contributor

Lms24 commented Oct 15, 2024

Describe the bug

When using our package @sentry/sveltekit in a minimal SvelteKit app with @sveltejs/adapter-cloudflare, a build error is logged when the adapter is invoked (see logs).

Upon closer inspection, it seems like the esbuild build within the cloudflare adapter is resolving the browser entry of our package's package.json exports instead of the server-side part. In the client-side part of our SDK package, we import { page, navigating } from "$app/stores" which esbuild can't resolve and therefore throws an error.

I already tried adding a worker exports condition in our package.json that would point to server files but this doesn't do anything. It still seems like esbuild is accessing our bowser exports entry point.

Reproduction

I created a minimal reproduction - please feel free to clone it

Logs

> Using @sveltejs/adapter-cloudflare
✘ [ERROR] Could not resolve "$app/stores"

    node_modules/@sentry/sveltekit/build/esm/client/browserTracingIntegration.js:1:33:
      1 │ import { page, navigating } from '$app/stores';~~~~~~~~~~~~~

  You can mark the path "$app/stores" as external to exclude it from the bundle, which will remove this error and leave the unresolved path in the bundle.


error during build:
Error: Bundling with esbuild failed with 1 error
    at adapt (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/@sveltejs/adapter-cloudflare/index.js:140:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async adapt (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/@sveltejs/kit/src/core/adapt/index.js:38:2)
    at async finalise (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:891:7)
    at async Object.handler (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:921:5)
    at async PluginDriver.hookParallel (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/rollup/dist/es/shared/node-entry.js:20652:17)
    at async Object.close (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/rollup/dist/es/shared/node-entry.js:21627:13)
    at async build (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/vite/dist/node/chunks/dep-Cyk9bIUq.js:65455:17)
    at async CAC.<anonymous> (file:///Users/lukas/code/test-projects/gh-sentry-javascript-13976-sveltekit-cloudflare-pages/my-svelte-app/node_modules/vite/dist/node/cli.js:828:5)

System Info

System:
    OS: macOS 14.7
    CPU: (10) arm64 Apple M1 Pro
    Memory: 103.63 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.volta/tools/image/node/20.11.0/bin/node
    Yarn: 4.1.1 - ~/.volta/tools/image/yarn/4.1.1/bin/yarn
    npm: 10.4.0 - ~/.volta/tools/image/npm/10.4.0/bin/npm
    pnpm: 8.14.3 - ~/.volta/tools/image/pnpm/8.14.3/bin/pnpm
    bun: 1.1.26 - ~/.bun/bin/bun
    Watchman: 2024.07.15.00 - /opt/homebrew/bin/watchman
  Browsers:
    Brave Browser: 101.1.38.111
    Chrome Canary: 123.0.6268.0
    Edge: 129.0.2792.89
    Safari: 17.6
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.0 => 3.2.5 
    @sveltejs/adapter-cloudflare: ^4.7.3 => 4.7.3 
    @sveltejs/kit: ^2.0.0 => 2.7.1 
    @sveltejs/vite-plugin-svelte: ^4.0.0-next.6 => 4.0.0-next.8 
    svelte: ^5.0.0-next.1 => 5.0.0-next.265 
    vite: ^5.0.3 => 5.4.9 

Severity

annoyance

Additional Information

⬆️ RE Severity: Blocker for users who'd like to add Sentry to their SvelteKit app running on Cloudflare

I'd appreciate any pointers as to what we/library authors need to do to support a use case where

  • the package exports files for both, browser and server
  • the browser-side code imports from kit-specific modules like $app/stores
  • the cloudflare adapter is used

Thanks a lot and please let me know if I can provide more information!

@eltigerchino
Copy link
Member

eltigerchino commented Oct 18, 2024

Hi Lukas, thanks for filing this issue. I'm not too knowledgeable on this but I tried adding $app/* to the esbuild external array to see if the build would pass. Then, I got the error where you mentioned it needed to import some server-side code:

✘ [ERROR] No matching export in "node_modules/@sentry/sveltekit/build/esm/index.client.js" for import "sentryHandle"

    .svelte-kit/output/server/chunks/hooks.server.js:10:9:
      10 │ import { sentryHandle, handleErrorWithSentry } from "@sentry/svelt...
         ╵          ~~~~~~~~~~~~


error during build:
Error: Bundling with esbuild failed with 1 error
    at adapt (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/@sveltejs/adapter-cloudflare/index.js:140:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async adapt (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/@sveltejs/kit/src/core/adapt/index.js:38:2)
    at async finalise (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/@sveltejs/kit/src/exports/vite/index.js:891:7)
    at async Object.handler (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/@sveltejs/kit/src/exports/vite/index.js:921:5)
    at async PluginDriver.hookParallel (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/rollup/dist/es/shared/node-entry.js:20652:17)
    at async Object.close (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/rollup/dist/es/shared/node-entry.js:21627:13)
    at async build (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/vite/dist/node/chunks/dep-Cyk9bIUq.js:65455:17)
    at async CAC.<anonymous> (file:///home/chewteeming/github/sentry-sveltekit-cloudflare-error/node_modules/vite/dist/node/cli.js:828:5)
 ELIFECYCLE  Command failed with exit code 1.

Could you try adding the worker exports condition in addition to making this change to the sveltekit cloudflare adapter and see if that works? In @sveltejs/adapter-cloudlfare/index.js:84:

-			const external = ['cloudflare:*', ...compatible_node_modules.map((id) => `node:${id}`)];
+			const external = ['cloudflare:*', ...compatible_node_modules.map((id) => `node:${id}`), '$app/*'];

Not sure if this is the right thing to do, but to my understanding those $app imports are virtual modules and don't need to bundled in anyway (unless I'm mistaken here). Also, I'm curious if we need to add this to other adapters or if it's just cloudflare

@Lms24
Copy link
Contributor Author

Lms24 commented Oct 18, 2024

I tried both, setting exports and updating the external array and I get the same error as you:

✘ [ERROR] No matching export in "node_modules/@sentry/sveltekit/build/esm/index.client.js" for import "sentryHandle"

    .svelte-kit/output/server/chunks/hooks.server.js:10:9:
      10 │ import { sentryHandle, handleErrorWithSentry } from "@sentry/svelt...

Which makes sense because sentryHandle is exclusively exported from index.server.js (it's a handler for the handle server hook).

I think the core issue still is that the esbuild build takes the wrong entry point for some reason.

For reference, here are the relevant parts of my package.json after adding the worker entries:

{
  "main": "build/cjs/index.server.js",
  "module": "build/esm/index.server.js",
  "browser": "build/esm/index.client.js",
  "worker": "build/cjs/index.server.js",
  "types": "build/types/index.types.d.ts",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "types": "./build/types/index.types.d.ts",
      "browser": {
        "import": "./build/esm/index.client.js",
        "require": "./build/cjs/index.client.js"
      },
      "worker": {
        "import": "./build/esm/index.server.js",
        "require": "./build/cjs/index.server.js"
      },
      "node": "./build/cjs/index.server.js"
    }
  }
}

As you can see, the only entries pointing to a client-side entry points are the browser ones.

I'd like to understand what the cloudflare adapter is doing with this esbuild command. Do you know what exactly it is building? I thought it's building a worker script but it doesn't really make sense to me why it'd take browser-side code for that. Maybe my understanding of the adapter is incorrect.

@eltigerchino
Copy link
Member

eltigerchino commented Oct 18, 2024

I'd like to understand what the cloudflare adapter is doing with this esbuild command. Do you know what exactly it is building? I thought it's building a worker script but it doesn't really make sense to me why it'd take browser-side code for that. Maybe my understanding of the adapter is incorrect.

To my understanding, it's bundling all the code and dependencies for the single worker that will be deployed. It uses the same conditions array as Cloudflare when they're bundling for the worker. https://github.com/cloudflare/workers-sdk/blob/a12b2786ce745f24475174bcec994ad691e65b0f/packages/wrangler/src/deployment-bundle/bundle.ts#L35-L36

@Lms24
Copy link
Contributor Author

Lms24 commented Oct 18, 2024

Sorry for the naive question but the single worker basically is the server-side part of a regular sveltekit build, correct?

@eltigerchino
Copy link
Member

Sorry for the naive question but the single worker basically is the server-side part of a regular sveltekit build, correct?

Yes, that's right.

@Lms24
Copy link
Contributor Author

Lms24 commented Nov 11, 2024

Given Svelte 5 is released, any chance for an update on this? Unfortunately this is still a blocker for us and all SvelteKit<>Cloudflare<>Sentry users.

@eltigerchino
Copy link
Member

eltigerchino commented Nov 11, 2024

Hi @Lms24 I tried re-arranging the exports order for @sentry/sveltekit so that it discovers the worker export before the browser one.

  "exports": {
    "./package.json": "./package.json",
    ".": {
      "types": "./build/types/index.types.d.ts",
      "worker": {
        "import": "./build/esm/index.server.js",
        "require": "./build/cjs/index.server.js"
      },
      "node": "./build/cjs/index.server.js",
      "browser": {
        "import": "./build/esm/index.client.js",
        "require": "./build/cjs/index.client.js"
      }
    }
  },

Unfortunately this produces 74 other build errors related to node module imports that can't be used in the workerd runtime.

As an aside, do you have a discord ID so that we can communicate directly?

@SG60
Copy link
Contributor

SG60 commented Nov 26, 2024

Hi, are there any more updates on this?

Also, I'm wondering whether sveltekit could re-use more of the Cloudflare Wrangler esbuild config to solve this. For example, this plugin, that handles the nodejs_compat flag:
https://github.com/cloudflare/workers-sdk/blob/02a0e1e186706eaec46048252068713f04698384/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

It seems like that might then solve the node module import errors that you are referring to @eltigerchino , and then the re-arranged export order will work.

@denny64

This comment was marked as duplicate.

SG60 added a commit to SG60/sentry-javascript that referenced this issue Dec 9, 2024
Needed together with changes to the Sveltekit Cloudflare Pages adapter, for sveltejs/kit#12813
@Lms24
Copy link
Contributor Author

Lms24 commented Dec 16, 2024

I tried your suggested exports change @eltigerchino and got to the same 74 build errors, basically all variations of

✘ [ERROR] Could not resolve "node:fs"

    node_modules/path-scurry/dist/esm/index.js:5:26:
      5 │ import * as actualFS from 'node:fs';
        ╵                           ~~~~~~~~~

Which I think is true for a native worker environment but if users enable

compatibility_flags = ["nodejs_compat"]

which they have to do anyway to use our CF SDK, these errors should be gone 🤔 However, it looks like this flag isn't respected by the build.

I see #13132 attempts to fix this so I'm looking forward to seeing how this works :)

@eltigerchino
Copy link
Member

We're still bundling in the Cloudflare workers adapter until #13072 is merged. Maybe we can separate that and the Workers Static Assets change into different PRs.

@benmccann
Copy link
Member

We're still bundling in the Cloudflare workers adapter until #13072 is merged. Maybe we can separate that and the Workers Static Assets change into different PRs.

Sure. I'll leave it to you guys. Whatever you prefer

@astralhpi astralhpi linked a pull request Feb 6, 2025 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment