adapters shouldn't pass arbitrary options to esbuild

[Rich-Harris]

See #4626 (comment) for a previous discussion about this. Currently, adapter-cloudflare and adapter-cloudflare-workers both pass their own options to esbuild.

This is bad API design — it's brittle and leaky. Beyond that, I'm not convinced it's even necessary. The cases where people want to manipulate the build output in some way (shimming node built-ins, etc) are all cases where the manipulation should probably happen long before the adapter gets involved. Assuming we manage to pull off #3535, that will become important anyway.

This PR removes the options altogether. If people report issues, we could consider adding the options back, but with a different API (like { esbuild: {...} }, or exposing individual options like external the way we do for adapter-vercel, not that I'd expect that to make any sense in the Cloudflare context).

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 8916210

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@sveltejs/adapter-cloudflare	Patch
@sveltejs/adapter-cloudflare-workers	Patch
@sveltejs/adapter-auto	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dominikg]

fyi @pzuraq also mentioned a need for external here #4276 (comment)

[f5io]

this would definitely be a breaking change for us, however I am going to investigate an alternative approach that encapsulates my build requirements at the vite configuration level. after initial investigation im not 100% sure this is going to be possible.

ideally for myself, exposing a unified interface for all adapters that allows people to hook into the esbuild process is extremely useful for getting out of some sticky jams to do with your target environment. my vote goes for the shape @Rich-Harris hinted at above, something like { esbuild: {...} }.

i will update here once ive been able to fully test if we can achieve what we need without this requirement

[pzuraq]

I would also strongly prefer the esbuild option for the time being. I think long term I agree that the builds should ideally be the same for every environment, that's the long term goal here and we're getting closer all the time as libraries adopt native crypto, modules, build processes, etc. But there are still edge cases that cause things to just fail and until the ecosystem has gotten a bit further along it would be best to have the option to customize builds somehow, IMO.

[Rich-Harris]

exposing a unified interface for all adapters that allows people to hook into the esbuild process is extremely useful

Many adapters don't use esbuild at all, and then you have things like adapter-vercel which will soon offer an option to choose between lambda (no esbuild required) and edge (esbuild required), initially on a per-deployment basis but eventually possibly on a per-route basis. So it's not clear to me that a unified interface is a realistic or desirable goal. This, to me, is more evidence that workarounds for troublesome libraries belong in the Vite config.

If someone can provide a repro of a problem that is currently solved via esbuild config passed to an adapter that couldn't be solved through Vite config, I'd love to take a look.

[f5io]

makes sense, just waiting on the pre-rendering fix to land before we migrate our apps to the Module Worker pattern

[Rich-Harris]

I just cut a new release

[pzuraq]

This could be related to the way adapter-cloudflare-workers calls ESBuild then, but in our usage many different options don't make it through from vite. external and define don't, for instance, make it into the final build, which is why we have to customize the options in the adapter.

I'll try to make a reproduction when I have time.

[f5io]

after a few hours of messing around with custom rollup/vite plugins, i've been unable to get a complete solution for the stripe node package to work in adapter-cloudflare-workers without the ability to inject esbuild options into the adapter.

i got mostly there, resolving the node internal packages to browserify alternatives. however, it appears i'm hitting a similar issue to @pzuraq. no matter what i try, the define option provided to vite.esbuild.define in my svelte.config.js does not replace process.env references as i need it to. i went so far as to try a custom vite plugin which transforms the code, yet even the transformed code does not appear in the build output from a svelte-kit build, yet i know my code runs due to logs appearing during build.

i feel like i'm close, but i also feel like it's a set of magic incantations to get this to work through vite. you can see my attempts below.

vite plugin (shimming node built-ins) - as far as i can tell this works

import { resolve, join } from 'path';
import findWorkspaceRoot from 'find-yarn-workspace-root';

const root = findWorkspaceRoot(process.cwd());
const nm = join(root, 'node_modules');

export function stripePre() {
  return {
    name: 'vite-plugin-stripe-cloudflare-workers',
    enforce: 'pre',
    resolveId(importee, ...rest) {
      if (
        importee.startsWith('\0child_process')
        || importee.startsWith('\0http')
        || importee.startsWith('\0https')
      ) {
        return {
          id: resolve(nm, '@package/build-tools/stub.js'),
        };
      }

      if (importee.startsWith('\0buffer')) {
        return {
          id: resolve(nm, 'buffer/index.js'),
        };
      }

      if (importee.startsWith('\0crypto')) {
        return {
          id: resolve(nm, 'crypto-browserify/index.js'),
        };
      }

      if (importee.startsWith('\0events')) {
        return {
          id: resolve(nm, 'events/events.js'),
        };
      }

      if (importee.startsWith('\0path')) {
        return {
          id: resolve(nm, 'path-browserify/index.js'),
        };
      }

      if (importee.startsWith('\0stream')) {
        return {
          id: resolve(nm, 'stream-browserify/index.js'),
        };
      }

      if (importee.startsWith('\x00util')) {
        return {
          id: resolve(nm, 'util/util.js'),
        };
      }
    },
  };
}

vite plugin (overwriting references to process.env) - this is not showing up in output, however it _is_ run. this is simply an alternative trying to make `define`

export function stripePost() {
  return {
    name: 'vite-plugin-stripe-cloudflare-workers-post',
    enforce: 'post', 
    transform(code, id) {
      let transformed = false;
      let output = code;
      if (output.includes('process.env.NODE_ENV')) {
        output = output.replaceAll('process.env.NODE_ENV', '"production"');
        transformed = true;
      }
      if (output.includes('process.env.NODE_DEBUG')) {
        output = output.replaceAll('process.env.NODE_DEBUG', 'false');
        transformed = true;
      }
      if (transformed) return { code: output, map: null };
    }
  }
}

i also still need to work out injection of globals, these are unfortunately globals required by stripe node dependencies, so i'm not sure how i would achieve that with using the hook.ts file in sveltekit.

[mglikesbikes]

@f5io — you might find this useful: cloudflare/workers-sdk#869

[Rich-Harris]

Added a check that immediately throws an error if someone passes esbuild options, and sends them here — hopefully that will flush out any cases where people are relying on these options, and we can come up with alternative (Vite-centric) solutions