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

chore(cloudflare): optimize Cache usage; strict TS source #4453

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Mar 25, 2022

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.

Rely on the Cache API sooner within adapter-cloudflare. Makes use of worktop for all Cache operations, which safely writes a Response into the Cache only when permitted.

Also converted the _worker.ts file to TypeScript for strict type checking, which is doable since we build with esbuild anyway.

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
Copy link

changeset-bot bot commented Mar 25, 2022

🦋 Changeset detected

Latest commit: c0724b2

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-cloudflare 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

@lukeed
Copy link
Member Author

lukeed commented Mar 25, 2022

Raised internally – i think the Pages application is misconfigured. It has:

[build.command]
pnpm build --filter="@sveltejs/kit" --filter="@sveltejs/adapter-cloudflare" --filter="@sveltejs/adapter-auto" --filter="create-svelte" --filter="default-template

[output]
packages/create-svelte/templates/default/.svelte-kit/cloudflare

but that output directory doesnt exist when run locally. True for the default branch too

@lukeed
Copy link
Member Author

lukeed commented Mar 28, 2022

CI is passing. Pages app seems unrelated

@Rich-Harris
Copy link
Member

The .svelte-kit/cloudflare output directory does exist locally if you set CF_PAGES=1 (which is how adapter-auto detects that it needs to delegate to adapter-cloudflare). The build failure is being caused by this...

13:46:21.752 | packages/create-svelte/templates/default build: > Using @sveltejs/adapter-cloudflare
-- | --
13:46:21.753 | packages/create-svelte/templates/default build:   Detected environment: Cloudflare Pages. Using @sveltejs/adapter-cloudflare
13:46:21.780 | packages/create-svelte/templates/default build: ✘ [ERROR] Could not resolve "worktop/cfw.cache"
13:46:21.780 | packages/create-svelte/templates/default build:     .svelte-kit/cloudflare-tmp/_worker.ts:3:23:
13:46:21.781 | packages/create-svelte/templates/default build:       3 │ import * as Cache from 'worktop/cfw.cache';
13:46:21.781 | packages/create-svelte/templates/default build:         ╵                        ~~~~~~~~~~~~~~~~~~~
13:46:21.781 | packages/create-svelte/templates/default build:   You can mark the path "worktop/cfw.cache" as external to exclude it from the bundle, which will remove this error.

...which makes me think it is related to this PR

@lukeed
Copy link
Member Author

lukeed commented Mar 31, 2022

Ok so then it must be a resolution thing? Because running build w/o the CF_PAGES=1 env is successful.

But yea, can now reproduce if I repeat with CF_PAGES present

@lukeed
Copy link
Member Author

lukeed commented Mar 31, 2022

Ah. Fails because the default template is built from within the create-svelte package (packages/create-svelte/templates/default/.svelte-kit/cloudflare-tmp) and the worktop package isn't resolvable from that location since root & create-svelte don't have it installed. This would be true for any runtime dependency, which none of the other adapters currently have.

@Rich-Harris Rich-Harris mentioned this pull request Mar 31, 2022
@Rich-Harris
Copy link
Member

Of course, that makes sense. In other adapters, we solve this problem by bundling earlier. We could do that here: #4473

* bundle worktop

* build on prepublishOnly

* Update packages/adapter-cloudflare/tsconfig.json

* update builder file paths

Co-authored-by: Luke Edwards <luke.edwards05@gmail.com>
Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but I'll confess I don't really understand why we need this to be a .ts file. I get that it doesn't make a huge difference seeing as we're bundling with esbuild, but I think there's probably some value in using .js consistently across the entire repo, rather than introducing an exception. Opened #4474

@lukeed
Copy link
Member Author

lukeed commented Mar 31, 2022

I don't really care about *.ts or not – esbuild is already there so it's free either way. I just want the strict types loaded to further prevent type & usage errors we saw in the originating Module Worker transform PR

@Rich-Harris Rich-Harris merged commit 316943f into master Mar 31, 2022
@Rich-Harris Rich-Harris deleted the chore/cloudflare-worktop branch March 31, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants