cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.17k stars 293 forks source link

Support class exports or clarify error message #490

Open jed opened 1 year ago

jed commented 1 year ago

One nice pattern I've used for Durable Objects is to implement the eyeball fetch as a static method on the object's class, ie:

class Host {
    static fetch(request, env) {
        let {hostname} = new URL(request.url)
        let hostId = env.hosts.idFromName(hostname)
        return env.hosts.get(hostId).fetch(request)
    }

    fetch(request) {
        return new Response('ok')
    }
}

Currently I can use this pattern like this:

export {Host}
export default {fetch: Host.fetch}

but ideally I'd be able to export the class itself, since it meets the requirements of being an object with a fetch property:

export {Host, Host as default}

workerd serve accepts this, but then fails at runtime with the following error:

workerd/io/worker-entrypoint.c++:210: error: e = kj/async-io-unix.c++:1651: failed: connect() blocked by restrictPeers()

I feel like workerd should either support this kind of export, or fail at compile time with a clearer error.

kentonv commented 1 year ago

So there are two issues here.

  1. The bad error message is because workerd has concluded that there is no fetch handler in the default entrypoint. In the absence of a fetch handler, it falls back to "default behavior", which is to "forward to origin", that is it behaves like the fetch handler simply did return fetch(request). That then gets blocked presumably because the request names localhost in the URL, and workerd blocks requests to the private/local network by default to prevent SSRF. Obviously, this "default behavior" is nonsensical for workerd. It's kind of nonsensical for modules syntax, even -- it's really a holdover from service workers.

  2. The runtime believes that if you export a class (or, technically, any function), it's strictly intended to be a durable object class. The runtime does not even check if it has a fetch member (I think), since it's a class. Off the top of my head, I can't think of a technical reason why we couldn't check for static members and thus allow a single entrypoint to be used for both DO and non-DO events. But, it sort of makes me nervous, like we might run into some challenge this causes in the future. And meanwhile, while I personally find it clever, I bet this is the kind of thing most developers will think is weird and confusing. I'll have to think more about this.