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

Support withCache in debug-network for workerd #1438

Merged
merged 2 commits into from
Oct 22, 2023

Conversation

frandiox
Copy link
Contributor

We can't use AsyncLocalStorage in Workerd to pass down request-id and other information. This PR adds new parameters to createWithCache to manually pass the required information down so that we can show calls to withCache in /debug-network.

Currently passing request but we can change it to headers or something else. Waiting on another conversation before deciding the final API.

@frandiox frandiox requested a review from a team October 19, 2023 07:08
};

function getHeader(key: string, request?: CrossRuntimeRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't request just type Request?

Copy link
Contributor Author

@frandiox frandiox Oct 19, 2023

Choose a reason for hiding this comment

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

In Oxygen yes but in Node environments they will be passing IncomingMessage (import type {IncomingMessage} from 'node:http'), I think? And the headers there doesn't have a get function, you just read like in an object.

I assume Bun and Deno probably have similar APIs for requests, either Web-like or Node-like, but maybe with different type name.

@@ -13,7 +13,8 @@ export default {
const cache = await caches.open('my-cms');
const withCache = createWithCache({
cache,
waitUntil: executionContext.waitUntil,
waitUntil: executionContext.waitUntil.bind(executionContext),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we losing this context without the bind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we already use .bind in all our templates so I just updated it here as well.

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

If we embrace supporting cross platform request objects, and have the generic getHeader function, I wonder if we could also get rid of getStorefrontHeaders, and just pass in a request object to createStorefrontClient.

@frandiox
Copy link
Contributor Author

frandiox commented Oct 22, 2023

If we embrace supporting cross platform request objects, and have the generic getHeader function, I wonder if we could also get rid of getStorefrontHeaders, and just pass in a request object to createStorefrontClient.

Yeah we could probably do that. Originally we went for just passing headers to avoid the cross platform request but I guess it's nicer DX at the end, and it gives us more flexibility.

Actually, I just remembered there's another constraint in createStorefrontClient. We need to buyer IP header, and that one in Oxygen is passed in the oxygen-buyer-ip but we don't know where it might be in other runtimes. So I guess storefrontHeaders is still useful because it allows you to pass buyerIp directly.

This is also kind of true for the request-id header, but at least that one doesn't have oxygen- prefix so anyone can add it to a Node request...

@frandiox frandiox merged commit 945c55a into main Oct 22, 2023
9 checks passed
@frandiox frandiox deleted the fd-debug-network-workerd-withcache branch October 22, 2023 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants