Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Removing shim from D1 #628

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Removing shim from D1 #628

merged 1 commit into from
Jul 18, 2023

Conversation

geelen
Copy link
Contributor

@geelen geelen commented Jul 13, 2023

Now that workerd includes the D1 shim, we no longer need wrangler to bundle it, and no longer require all bindings to be prefixed with __D1_BETA__.

This will need to be released alongside the Wrangler version that drops support for the shim as well, currently being tracked in this PR: cloudflare/workers-sdk#3595

@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2023

⚠️ No Changeset found

Latest commit: 23701d9

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.

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

@geelen geelen changed the base branch from master to tre July 13, 2023 12:34
@geelen geelen force-pushed the glen/d1-no-shim branch from 69369a8 to 2c03c79 Compare July 13, 2023 23:44
@geelen geelen marked this pull request as ready for review July 13, 2023 23:45
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Nice! 🥳 Couple minor comments...

@@ -1,11 +1,11 @@
# D1 Worker Fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason we had this fixture was to get the D1 shim from Wrangler for testing. miniflareTest() (see test/plugins/d1/index.spec.ts) can accept a request handler function like:

const test = miniflareTest({}, async (global, req) => {
// Partition headers
let name: string | undefined;
let cfCacheKey: string | undefined;
let bufferPut = false;
const reqHeaders = new global.Headers();
const resHeaders = new global.Headers();
for (const [key, value] of req.headers) {
const lowerKey = key.toLowerCase();
if (lowerKey === "test-cache-name") {
name = value;
} else if (lowerKey === "test-cf-cache-key") {
cfCacheKey = value;
} else if (lowerKey === "test-buffer") {
bufferPut = true;
} else if (lowerKey.startsWith("test-response-")) {
resHeaders.set(lowerKey.substring("test-response-".length), value);
} else {
reqHeaders.set(lowerKey, value);
}
}
// Get cache and cache key
const cache =
name === undefined ? global.caches.default : await global.caches.open(name);
const key = new global.Request(req.url, {
headers: reqHeaders,
cf: cfCacheKey === undefined ? undefined : { cacheKey: cfCacheKey },
});
// Perform cache operation
if (req.method === "GET") {
const cachedRes = await cache.match(key);
return cachedRes ?? new global.Response("<miss>", { status: 404 });
} else if (req.method === "PUT") {
const body = bufferPut ? await req.arrayBuffer() : req.body;
const res = new global.Response(body, { headers: resHeaders });
await cache.put(key, res);
return new global.Response(null, { status: 204 });
} else if (req.method === "DELETE") {
const deleted = await cache.delete(key);
return new global.Response(null, { status: deleted ? 204 : 404 });
} else {
return new global.Response(null, { status: 405 });
}
});

This handler gets wrapped with all the MF-Experimental-Error-Stack stuff automatically too. Could tidy this up in a future PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just reusing the existing test file, before Wrangler had compiled it. What do you think?

packages/miniflare/src/plugins/d1/index.ts Outdated Show resolved Hide resolved
@geelen geelen force-pushed the glen/d1-no-shim branch 2 times, most recently from c89ae12 to 9b74ecc Compare July 18, 2023 03:27
@geelen geelen force-pushed the glen/d1-no-shim branch from 9b74ecc to 23701d9 Compare July 18, 2023 03:44
@geelen
Copy link
Contributor Author

geelen commented Jul 18, 2023

Updated the PR to make Miniflare backwards-compatible with the older style of bindings.

I also broke the test apart into a shared.ts, which has the basic test infra, and then a suite.ts, since the two test cases differ only on which file they're including, and the binding name.

Not sure this is great but for right now, all I want to confirm is that the runtime shim has the exact same behaviour as the Wrangler-based shim...

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

@mrbbot mrbbot merged commit 9b8eb97 into cloudflare:tre Jul 18, 2023
@geelen geelen deleted the glen/d1-no-shim branch July 18, 2023 23:14
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants