Missing EventTarget and Event

[jayphelps]

For compliance with Cloudflare the global object (aka globalThis et al) and WebSocket need to inherit from EventTarget and both EventTarget and Event need to be exposed globally. I did some preliminary investigation and it’s not trivial to do; WebSocket implementation etc. but it seems doable.

I don’t need this right this second, so no immediate rush, but it was discovered while trying to test our complex worker so if/when we eventually need this we’ll spend more time and potentially contribute the fixes, assuming you haven’t prioritized it yourself. Our usage is pretty advanced, so I imagine most won’t run into this.

[mrbbot]

👋 Hey. As you say this should be doable, I'll investigate what's needed this weekend if I get some time. 🙂

[mrbbot]

Hello again! I've spent a bit of time investigating this.

If we were going to implement this, ideally we'd use NodeJS's built-in EventTarget and Event classes, but these were only exposed to user code in Node 15 (to be honest this is the case for Web Streams and Web Crypto too). For version 1 of Miniflare at least, I'm keen to continue supporting supported LTS versions of NodeJS, so this isn't great.

There are existing shims for these though (e.g. https://github.com/mysticatea/event-target-shim) that looked pretty good so I started to play around with integrating these.

Do you know how much of EventTarget Cloudflare supports on the global object? What features of it are you using? When I try to run the following examples with wrangler dev, I get a 400 Bad Request error (same error as uploading an empty file with no event listeners):

addEventListener("fetch", {
  handleEvent: function(event) {
    event.respondWith(new Response("test"));
  },
});

addEventListener(
  "fetch",
  (event) => {
    event.respondWith(new Response("test"));
  },
  { once: true }
);

I guess the once example doesn't really make sense for a fetch event. Haven't tried with WebSockets but imagine it would work there.

[jayphelps]

I’ll reply with more thorough answer later but indeed we ran into missing features of EventTarget, including ‘once’

[jayphelps]

As wild and confusing as it is, they also have TWO copies of EventTarget: one that is the parent prototype of the global, and another for the EventTarget global variable. That means it’s obv not spec compliant. https://community.cloudflare.com/t/serviceworkerglobalscope-doesnt-inherit-from-the-same-eventtarget-as-globalthis-eventtarget/266923

[jayphelps]

There’s also generally a question around how much miniflare wants to be 1:1 compatible when it comes to Cloudflare being incomplete or having bugs. Eg miniflare supports custom ReadableStreams via constructor start(), Cloudflare does not. If the goal is exclusively to give a testing ground for Cloudflare workers, it makes sense to emulate those limitations indeed, but that’s somewhat unfortunate as miniflare could have actually evolved into a true replacement instead, for prod, though presumably there would need to be many more things to get to that point—I imagine it’s out of scope.

[mrbbot]

It's a good question. Initially, the primary goal of Miniflare was a good developer (and testing) experience for workers. In that respect, as you say it would make sense to emulate those limitations. I would like to think Cloudflare would eventually address them. 😄

It would definitely be cool to evolve into a true replacement, things like Redis support for KV/Durable Objects/Cache are a step towards that, but Miniflare was still designed to run as a single instance, which would probably be unsuitable for prod.

[lukeed]

Generally I wouldn't recommend this, but for a project like miniflare, it could make sense to require Node 15 or Node 16 at minimum support. That way can take advantage of the webcrypto, webstreams, and now EventTarget natives instead of shipping polyfills.

This suggestion would be a breaking change though, so would require a 2.0 release if it were to happen.

[mrbbot]

Implemented this in f97412d for version 2, which will require Node 16. 👍

Had to do something a little funky to handle event listeners that throw exceptions. On Cloudflare, throwing in an event listener stops all further listeners running and returns an error page. We'd like to do the same thing, and show the pretty error page too. This behaviour is also important to handle ​passThroughOnException properly. By default, Node's EventTarget raises uncaught exceptions on listener throw, so I wrap all event listeners with an additional try/catch that records errors and calls stopImmediatePropagation on the event:

https://github.com/mrbbot/miniflare/blob/f97412d36e9e45d703e62fcef80038b0dca7c2e6/src/modules/events.ts#L119

Then in dispatchEvent, if an error is thrown, we rethrow it. This is only implemented for ServiceWorkerGlobalScope, not WebSocket:

https://github.com/mrbbot/miniflare/blob/f97412d36e9e45d703e62fcef80038b0dca7c2e6/src/modules/events.ts#L183

Miniflare unintentionally implements more of the spec than Cloudflare at the moment too, which is both good and bad, but probably bad given the intended use case of testing workers before deploying.

[mrbbot]

I've backported this to 1.4.0 using event-target-shim. Version 2 will use Node's built-ins. 👍

[jayphelps]

Thank you!