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

feat: implement adapter.emulate #11732

Merged
merged 18 commits into from
Mar 12, 2024
Merged

feat: implement adapter.emulate #11732

merged 18 commits into from
Mar 12, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jan 24, 2024

Breaking this out of #11730, as the changes to adapter-cloudflare and adapter-cloudflare-workers are blocked on the next release of wrangler. Without it, we'll have to make platform.caches optional, which would be an annoying breaking change

(Actually, I've just realised that this technically only applies to adapter-cloudflare-workers, since context and caches are already optional in adapter-cloudflare for some reason. But it would be nice to support these in both.)


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:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jan 24, 2024

⚠️ No Changeset found

Latest commit: 62b9db2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rich-Harris
Copy link
Member Author

Rich-Harris commented Jan 24, 2024

TODO #11730 (comment)

@eltigerchino eltigerchino linked an issue Jan 25, 2024 that may be closed by this pull request
@OJFord
Copy link

OJFord commented Jan 27, 2024

wrangler@3.25.0 was released yesterday with a miniflare bump, is that the one we're waiting for?

@hrueger
Copy link

hrueger commented Jan 27, 2024

I think we're waiting for cloudflare/workers-sdk#4847

@OJFord
Copy link

OJFord commented Jan 30, 2024

That's landed in 3.26.0, which is available (pre-release) at https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7714710243/npm-package-wrangler-4854

I might try to work out how later anyway, but if it's of any help at all I'd be happy to give it a go to test.

@dario-piotrowicz
Copy link
Contributor

wrangler 3.26.0 (with the no-op caches implementation) has just been released 🙂

@OJFord
Copy link

OJFord commented Feb 10, 2024

I might try to work out how later anyway, but if it's of any help at all I'd be happy to give it a go to test.

I updated my dependencies to:

    "@sveltejs/adapter-cloudflare": "file:../kit/packages/adapter-cloudflare",
    "@sveltejs/kit": "^2.4.3",
    "wrangler": "^3.26.0",

cloned gh:sveltejs/kit --depth=1 --branch=cloudflare-adapter-dev, and ran:

../kit$yay -Sy pnpm # (install pnpm, whatever package manager)
../kit$pnpm install
../kit$pnpm build

removed my if-dev-then-miniflare hacks, and... it seems to be working fine - I'm only using D1 at the moment, but it seems to work exactly as promised; the only local-conditional stuff I have now is to make Drizzle play ball.

Thanks!

packages/adapter-cloudflare-workers/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare-workers/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare-workers/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare-workers/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare-workers/package.json Outdated Show resolved Hide resolved
packages/adapter-cloudflare/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare/package.json Outdated Show resolved Hide resolved
@ottomated
Copy link
Contributor

Is there anything blocking this at the moment? This would be a huge win for my productivity :)

@eltigerchino
Copy link
Member

eltigerchino commented Feb 29, 2024

@dario-piotrowicz @Rich-Harris is there anything else that needs to be done in this PR? Or does it look good enough after updating to the latest wrangler version and passing in the proxy properties to emulate?

benmccann and others added 4 commits February 29, 2024 10:46
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
@dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz @Rich-Harris is there anything else that needs to be done in this PR? Or does it look good enough after updating to the latest wrangler version and passing in the proxy properties to emulate?

I think it looks great! 😄
(with your suggestions applied that is)

From my point of view it looks good and could be shipped 🙂, we can always iterate and improve things if the need arises

Co-authored-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>
@benmccann
Copy link
Member

Type checking is currently failing because some required fields on platform are not defined in adapter-cloudflare-workers. I notice these fields are are required in adapter-cloudflare-workers/ambient.d.ts and not in adapter-cloudflare/ambient.d.ts, which is why it's only the adapter-cloudflare-workers build that's failing. I'm not familiar enough with this functionality to know if we want those fields to be required or not, but it seems like we should align them

@benmccann benmccann marked this pull request as ready for review March 1, 2024 17:09
@eltigerchino
Copy link
Member

eltigerchino commented Mar 2, 2024

I think it would make sense to make all of the properties non-optional since they're now provided during dev and build. They should be provided during prerendering as well, but throw an error similar to how we handle accessing env during prerendering in this PR. Should these type changes be made in a separate PR?

ambient.d.ts

import { CacheStorage, IncomingRequestCfProperties } from '@cloudflare/workers-types';

declare global {
	namespace App {
		export interface Platform {
+			/**
+			 * Lifecycle methods to augment or control how the request is handled.
+			 */
-			context?: {
+			context: {
+				/**
+				 * Executes a promised-based task before the handler terminates but without blocking the response.
+				 * 
+				 * See the [official docs](https://developers.cloudflare.com/workers/runtime-apis/handlers/fetch/#contextwaituntil).
+				 * 
+				 * @param promise
+				 */
				waitUntil(promise: Promise<any>): void;
			};
+			/**
+			 * The Cache API allows fine grained control of reading and writing from the Cloudflare global network cache.
+			 * 
+			 * See the [official docs](https://developers.cloudflare.com/workers/runtime-apis/cache/).
+			 */
-			caches?: CacheStorage;
+			caches: CacheStorage;
+			/**
+			 * An object containing properties about the incoming request provided by Cloudflare’s global network.
+			 * 
+			 * See the [official docs](https://developers.cloudflare.com/workers/runtime-apis/request/#incomingrequestcfproperties).
+			 */
-			cf?: IncomingRequestCfProperties;	
+			cf: IncomingRequestCfProperties;	
		}
	}
}

The only issue is that they're not available during preview (but we want that to proxy to the wrangler command where they are available). Otherwise, making them all optional would be a breaking change for adapter-cloudflare-workers no?

@benmccann benmccann merged commit cb338a6 into main Mar 12, 2024
11 of 13 checks passed
@benmccann benmccann deleted the cloudflare-adapter-dev branch March 12, 2024 15:37
eltigerchino added a commit that referenced this pull request Mar 13, 2024
* add changeset

* documentation

* Update documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

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

* Update documentation/docs/25-build-and-deploy/70-adapter-cloudflare-workers.md

Co-authored-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>

---------

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>
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.

Platform context fallbacks
8 participants