Pass `external` to esbuild in the Cloudflare adapters by cimnine · Pull Request #10424 · sveltejs/kit

cimnine · 2023-07-23T09:40:54Z

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, such as fs. This causes problems during npm run build, even if my code does not rely on the respective code-path that requires any problematic modules (like fs). For me, this is the case with ical-generator.

This change is inspired by the already existing functionality in the Vercel adapter, that accomplished the same:

kit/packages/adapter-vercel/index.d.ts

Lines 56 to 75 in b251b23

    
           export interface EdgeConfig { 
        
           	/** 
        
           	 * Whether to use [Edge Functions](https://vercel.com/docs/concepts/functions/edge-functions) or [Serverless Functions](https://vercel.com/docs/concepts/functions/serverless-functions) 
        
           	 */ 
        
           	runtime?: 'edge'; 
        
           	/** 
        
           	 * To which regions to deploy the app. A list of regions or `'all'`. 
        
           	 * More info: https://vercel.com/docs/concepts/edge-network/regions 
        
           	 */ 
        
           	regions?: string[] | 'all'; 
        
           	/** 
        
           	 * List of packages that should not be bundled into the Edge Function. 
        
           	 * Edge only. 
        
           	 */ 
        
           	external?: string[]; 
        
           	/** 
        
           	 * If `true`, this route will always be deployed as its own separate function 
        
           	 */ 
        
           	split?: boolean; 
        
           }

Respectively:

kit/packages/adapter-vercel/index.js

Lines 111 to 124 in b251b23

    
           await esbuild.build({ 
        
           	entryPoints: [`${tmp}/edge.js`], 
        
           	outfile: `${dirs.functions}/${name}.func/index.js`, 
        
           	target: 'es2020', // TODO verify what the edge runtime supports 
        
           	bundle: true, 
        
           	platform: 'browser', 
        
           	format: 'esm', 
        
           	external: config.external, 
        
           	sourcemap: 'linked', 
        
           	banner: { js: 'globalThis.global = globalThis;' }, 
        
           	loader: { 
        
           		'.wasm': 'copy' 
        
           	} 
        
           });

The only related issue that I've found was #6547, which was resolved because Stripe changed their implementation. This is the ideal resolution, I admit, but the world is not ideal as far as I'm concerned.

changeset-bot · 2023-07-23T09:40:57Z

🦋 Changeset detected

Latest commit: 3c767a7

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 · 2023-07-23T13:56:13Z

Related
#10214
#10110

cimnine · 2023-07-23T15:56:40Z

Oh, I didn't check the PRs before 😅 At least, there seems to be a need.

cimnine · 2023-08-02T21:55:07Z

I've applied the requested changes and rebased onto the latest master.

benmccann · 2023-08-02T21:56:04Z

Related
#10214
#10110

Thanks for tracking those down. I've closed them in favor of this PR, which seems closest to being ready

cimnine · 2023-08-03T05:38:30Z

 };
 ```

+### Custom config


Should I change this one to config as well, like:

Suggested change

### Custom config

### `config`

That would be consistent with the other file, IMO.

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

benmccann · 2023-08-10T16:07:09Z

Closing this in favor of #10521

vercel Bot deployed to Preview – kit-default-template July 23, 2023 09:41 View deployment

cimnine marked this pull request as ready for review July 23, 2023 09:41

vercel Bot deployed to Preview – kit-default-template July 23, 2023 09:52 View deployment

benmccann reviewed Jul 24, 2023

View reviewed changes

Comment thread CONTRIBUTING.md Outdated

benmccann reviewed Jul 24, 2023

View reviewed changes

Comment thread documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md Outdated

benmccann mentioned this pull request Jul 27, 2023

feat(adapter-cloudflare): expose the external ESBuild option #10110

Closed

5 tasks

vercel Bot deployed to Preview – kit-default-template August 2, 2023 21:28 View deployment

vercel Bot deployed to Preview – kit-default-template August 2, 2023 21:49 View deployment