-
Notifications
You must be signed in to change notification settings - Fork 252
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
eventsource@v3: looking for testers #329
Comments
I use eventsource3, and the program will exit inexplicably after running for a few hours without any alarm (I open and close a different eventsource every minute). With eventsource2, there is no such problem. my code is like below
|
Under which environment is this? Node.js? Which version? I'm not familiar with the language above, but could you add a catch handler to your promise that awaits the espromise that logs any errors? |
i think event2 will log the error |
I am unable to reproduce. I've tried recreating something that follows your code here: https://github.com/rexxars/es-repro-329 You can switch between EventSource@v2 and EventSource@next by swapping the EventSource variable. When running this, I get (in both v2 and v3):
Are you able to modify my test repo code to recreate the case you are seeing? |
eventsource v3
TL;DR
I want to modernize this module, making it both easier to maintain and more cross-platform friendly.
You can help test the next version of this module by installing
eventsource@next
from npm, and reading the migration guide to see how to update your code.Any issues you find can be reported on this issue. The code for the new version is available in the v3 branch.
Background
This package is soon to be 13 years old. When Aslak and Einar created the first version of this module, node.js wasn't even at 0.10 yet. A lot has changed since those days:
fetch
API available in all modern browsers, Node.js, Deno, Bun etc. A recent addition is the "web streams" API - also available in the same environments.@types
module for TypeScript support.Proposal
Use ES Modules (ESM), with CommonJS fallback
We'll build the module using @sanity/pkg-utils (because we use it where I work, and I am familiar with it), and use conditional exports to provide both ESM and CommonJS variants. Will also switch to named exports, since it has a better cross-module system compatibility.
Split the parser and the client
I've made a parser module for EventSource that is minimal, well tested and works in "any" JavaScript environment. Splitting the concerns makes sense from a testing and maintenance perspective.
Use TypeScript for type safety
I find it easier to maintain and think about code when it's typed. Not having to keep the
@types/eventsource
module in sync with the main module is a big plus.Use fetch, ReadableStream, EventTarget
Allows us to reuse the same code across Node.js, browsers and other environments that support the
fetch
API. The polyfill for browsers becomes very slim, and there are few behavioral differences between the environments.Remove (most) request-based extensions
In v2, there are some extensions to the WhatWG spec - allowing users to pass custom headers, adding support for proxies, specifying HTTPS options etc. I propose that all of these are removed, and instead left up to the user to implement: they are trivially implemented by passing a custom
fetch
function. This creates a single extension point, making the module easier to maintain.Drop support for Node.js < 18
In order to use
fetch
, web streams and other modern APIs, we need to drop support for Node.js versions < 18. This is a tradeoff I'm willing to make, as Node 18 is going out of LTS in April 2024 and older versions are already out of LTS. Users who are on older versions can still rely on v2 of this module.Repo: Use conventional commits, automate releases
In order to make the module easier to maintain, I propose we switch to using conventional commits, and automate releases using GitHub Actions. This will make it easier to maintain the module, and make it easier for contributors to understand the release process. Automatic generation of changelogs is a nice bonus.
Repo: Move to
main
branchI propose we move the default branch from
master
tomain
. This better aligns with the new default branch naming in GitHub.Disclaimer
While these changes may seem drastic, they are necessary for me to continue supporting this module long term. It also falls within the bounds of what Aslak said when he passed on the maintainership of the module to me:
This is exactly what I am proposing, by encapsulating all/most non-standard extensions in a single extension point (the
fetch
property).While not 100% true, it only has a single dependency -
eventsource-parser
, which has zero dependencies of its own.This is probably the biggest change - in a positive way. Instead of having to rely on a webpacked file that sets some browser globals, we can now use the module directly in the browser, and with no node.js-polyfills bundled.
I will continue to welcome contributions, but also want to ensure that we keep an eye on security. 19+ million downloads per month means we have a responsibility to our users. By making releases built and signed by GitHub actions, the risk of malicious code being published by someone accidently gaining access to an npm account is reduced.
Preliminary test results
I've tried to exercise the new module by running a test suite that connects to a Server-Sent Events endpoint which emits events every ~10 seconds, sends comments as "keep-alives" (technically not needed given the 10s event interval) and occasionally closes the connection due to the load balancer in front of the endpoint terminating long-lived connections. The tests are run on Node.js, bun and in browsers. In server environments, I am testing the old
EventSource@2
implementation vs the new one, and in browsers I am running the new implementation vs the browser native implementation and vs the yaffle polyfill. I am not running it against the browser-compiled variant ofEventSource@2
since I consider it almost "an accident" that it works in browsers, given the amont of polyfills for node internals it uses.This is all running on my laptop on a wifi connection which tends to be online, but sometimes goes into slumber or drops the wifi or similar. The benefit here is that I get to test and compare both the "optimal" case as well as the "difficult" cases (wifi disconnect, computer goes into sleep mode) and see that things recover as expected.
The results are promising:
One thing I did discover with the browser variant of the new module is that the
Last-Event-ID
header is not a CORS safelisted header, and thus reconnections cross-origin causes a preflight request to check if allowed to send the header. It seems odd that the native EventSource is allowed to send this header but not a fetch request, and I'm now working on rectifying this in the WhatWG fetch spec. Until then, we have two options:lastEventId=<id>
), as well as the amvtek polyfill (evs_last_event_id=<id>
). Since this isn't standardized, I am not super eager to implement this - it could also be done manually by passing a customfetch
implementation which replaces thelast-event-id
header with a query string parameter of choice.The text was updated successfully, but these errors were encountered: