Pass alias and external to esbuild in the Cloudflare adapters

[cimnine]

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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Some external modules rely on NodeJS modules like fs. I've provided #10424, only to see that it's not enough. When running the worker, it will still try to load fs – and since CF does not provide it, the worker won't start properly. This happens despite my code not relying on the respective code-path that requires any problematic modules. For me, this is the case with ical-generator.

The alias option allows providing a stub implementation. In my case, I alias fs to my fs-stub.js, and this does the trick:

# fs-stub.js

export function writeFile() {
	console.error('writeFile() is not implemented');
}

export function writeFileSync() {
	console.error('writeFileSync() is not implemented');
}

export function promises() {
	console.error('promises() is not implemented');
}

This PR currently includes the changes of #10424. I could either split the PRs entirely, or we can merge just this one and close the other, or the other way round (since most of the review happened on the other PR already).

[changeset-bot]

🦋 Changeset detected

Latest commit: 5ac6b2a

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

This PR includes changesets to release 2 packages

Name	Type
@sveltejs/adapter-cloudflare-workers	Minor
@sveltejs/adapter-cloudflare	Minor

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

[gtm-nayan]

Is using both for the same package okay? What happens in this case?

[ghostdevv]

Looking at their docs it's essentially pointless as alias resolution happens first

[cimnine]

I was wondering whether I should move the samples for external and alias out of the top example section. I believe if people are new to SveltKit / to the adapter, they will just copy whatever example from the docs they can find to get going. And if that example contains the external and alias sections like defined above, this might have unintended consequences.

[gtm-nayan]

Thinking more about this, external would be for either modules that are loaded at runtime or to omit bundling something that won't be needed at runtime.

Something like node:buffer would be a good example for the first case but we have another PR to externalize all node: prefixed modules https://github.com/sveltejs/kit/pull/10544/files .

So we just have the second case, which would be something that's only dynamically imported/required on based on some condition.

[benmccann]

There's also a request to expose the esbuild platform or all esbuild options here: #9398

[ghostdevv]

If we are for this change I think we should close this in favour of #9398 to unify all adapters

[cimnine]

FYI: rebased onto latest master

[dummdidumm]

There's also #10544 which adds node: to external, which would also solve the "don't bundle fs" use case. I'm not 100% sure which one is better.

[cimnine]

@dummdidumm I believe I need the alias more, at least with Wrangler on localhost I did, otherwise the worker would not load. The external in my case only made the build succeed, but the worker wouldn't load in my case unfortunately.

[cimnine]

FYI: Rebased onto master

[benmccann]

I'll have to check with the other maintainers whether we want to do this, #9398, or neither. If we do add the ability to use esbuild options (either the subset here or all of them), I think we need to note that it is not recommended. The better solution in your case would be to tweak the ical-generator package as follows: sebbo2002/ical-generator#511. However, I recognize that some packages are unmaintained, etc. and so that solution is not always readily available. But at the very least I think we need to caveat any usage of these APIs with a disclaimer that you're introducing a workaround rather than addressing the root issue

[cimnine]

FYI: Rebased onto master

[cimnine]

I'll have to check with the other maintainers whether we want to do this, #9398

I think both options have their merits. The #9398 is definitely more flexible, but brings perhaps a little more maintenance efforts down the line, as users will expect all esbuild options to be available, so they will have to be made available in the type definition as they become available in esbuild.

The better solution in your case would be to tweak the ical-generator package as follows […]

In an ideal world, that'd be the way to go. However, the world is not ideal, …

However, I recognize that some packages are unmaintained, etc. and so that solution is not always readily available.

… as you recognize yourself. And maintaining a fork of these dependencies – or maintaining a fork of the respective SvelteKit adapters – is plenty of work, but will also introduce security issues down the line.

So I believe – looking at it globally – it's the smaller cost allowing users to add workarounds for issues in potentially many other modules in this single place, than in many other places and forks. (Even more so, since esbuild already has this options and it's largely exposing the specific options. They could also just have said «go fix upstream packages», but they didn't, probably for some reason.)

So I'd appreciate it if either this or #9398 would be included in a future release.

[cimnine]

FYI: Rebased onto master

[cimnine]

FYI: Rebased onto master

[nounder]

Any chance to merge it soon?

This would enable publishing to Cloudflare Workers (#3564)

2 pull requests have been already closed waiting for this PR to get merged. (#10424 #10214)

[Rich-Harris]

As of #10544, Node built-ins are supported when deploying to Cloudflare.

Things like fs-stub can (and arguably should) be handled with Vite:

/** @type {import('vite').UserConfig} */
const config = {
  plugins: [sveltekit()],

  resolve: {
    alias: {
      fs: path.resolve('./shims/fs.js')
    }
  }
};

As such, I believe this issue can be closed — any objections?

[nounder]

As such, I believe this issue can be closed — any objections?'

#10544 resolves node stubs issue for subset of supported node modules under nodejs_compat compatibility flag.

This PR enables to create aliases for monkey patching and partial stubs without enforcing use of compatibility flag.

I think this PR still should be merged.

[ghostdevv]

This PR enables to create aliases for monkey patching and partial stubs without enforcing use of compatibility flag.

Is this something that can't be achieved with vite like Rich shared?

[nounder]

@ghostdevv, you are right: Rich code works 👍 I think this PR is not necessary anymore.