feat: use worker bindings interacting from api to gateway #136
Conversation
Deploying with
|
| Latest commit: |
762c382
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f457bde5.nftstorage-link.pages.dev |
| Branch Preview URL: | https://fix-use-worker-bindings.nftstorage-link.pages.dev |
vasco-santos
force-pushed
the
fix/use-worker-bindings
branch
3 times, most recently
from
01390ed
to
e1ea575
Compare
vasco-santos
force-pushed
the
fix/use-worker-bindings
branch
from
e1ea575
to
089b833
Compare
vasco-santos
force-pushed
the
fix/use-worker-bindings
branch
from
089b833
to
3457956
Compare
vasco-santos
changed the title
vasco-santos
force-pushed
the
fix/use-worker-bindings
branch
from
3457956
to
03de54f
Compare
There was a problem hiding this comment.
The code changes look good, but I need a bit more context on what you know so far about this "app-specific-gateway worker to shared-gateway worker" pattern will get us.
Will it really get us a common cache? Or do we need to test it to find out?
If a bound worker is really just code bundled with another worker, I would guess that we still wouldn't benefit from a shared common cache, but each app-gateway-specific-gateway would just be writiing to it's own cache just using the shared code of the shared-gateway.
packages/api/wrangler.toml
Outdated
|
|
||
| [[env.production.r2_buckets]] | ||
| bucket_name = "super-hot" | ||
| binding = "SUPERHOT" | ||
|
|
||
| [[env.production.unsafe.bindings]] |
There was a problem hiding this comment.
are unsafe.bindings documented anywhere? Can you link to docs or add a note here about how they work and why they may or may not be unsafe?
There was a problem hiding this comment.
All i can find is this PR which mentions they are undocumented cloudflare/workers-sdk#1004
There was a problem hiding this comment.
and this... the list of drawbacks listed on this PR is scary
cloudflare/workers-sdk#906
There was a problem hiding this comment.
The unsafe is not needed anymore in fact. They already support without it :)
The drawbacks there seem outdated by know from what I have seen in their discord
packages/api/test/scripts/gateway.js
Outdated
| * @param {URL} url | ||
| */ | ||
| function getCidFromSubdomainUrl(url) { | ||
| // Replace "ipfs-staging" by "ipfs" if needed |
There was a problem hiding this comment.
Comment looks like a copy-pasta... or is there a deployment issue i'm not seeing
| // Replace "ipfs-staging" by "ipfs" if needed |
| [[env.production.unsafe.bindings]] | ||
| name = "GATEWAY" | ||
| type = "service" | ||
| service = "gateway-nft-storage-production" |
There was a problem hiding this comment.
do we have a naming scheme for our services? Should we define one?
There was a problem hiding this comment.
We probably should re-think this, but perhaps as part of the new workers read architecture. This name is the name of the already deployed nftstorage.link worker
vasco-santos
force-pushed
the
fix/use-worker-bindings
branch
from
48d2817
to
762c382
Compare
Let's talk sync on this. btw, this is not related at all with this PR 😛 This is basically using bindings between nftstorage.link gateway and API (for perma-cache) as part of #101 |
| @@ -222,12 +212,29 @@ function validateCacheControlHeader(cacheControl) { | |||
| * @param {Request} request | |||
| */ | |||
| function getHeaders(request) { | |||
| const existingProxies = request.headers.get('X-Forwarded-For') | |||
| ? `, ${request.headers.get('X-Forwarded-For')}` | |||
| const headers = cloneHeaders(request.headers) | |||
There was a problem hiding this comment.
note: this is needed for the range headers support PR comming later.
This PR adds:
Implementation details:
/etc/hosts, which is not cool, given we can't use tools like https://github.com/feross/hostile without sudo for pre and post tests... There was the option of relying on services like https://readme.localtest.me/ but don't really want to use external services in CINeeds:
Initial binding part of #101 to move to worker bindings setup