feat: Implement EventSource

[Uzlopak]

This is my attempt to implement eventsource into undici and thus into nodejs.

I am currently working on it, of course. EventSourceStream is not working perfect, so that would be my priority. After that I would focus on spec compliance.

But my biggest issue still needs clarification:

EventSource in a Browser UserAgent expects that you can use "withCredentials" option to send cookies with the request. So this is kind of limiting as I assume that node native fetch is not storing any cookies. So maybe we should deviate from the spec and allow maybe like npm:eventsource to pass headers via the options parameter. Thus you could also use the authorization header to connect to a EventSource server.change

@KhafraDev
I am looking forward for some hints regarding expected coding style in undici. E.g. should I replace the private properties to symbols?

Should I integrate wpt tests? Tbh. they are not working out of the box, and needs python.

Looking forward for your feedback.

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[codecov-commenter]

Codecov Report

Attention: 201 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (4b18785) 85.44%.
Report is 230 commits behind head on main.

Files	Patch %	Lines
lib/fetch/index.js	67.50%	52 Missing ⚠️
lib/fetch/util.js	38.46%	48 Missing ⚠️
lib/cache/cache.js	2.94%	33 Missing ⚠️
lib/handler/RetryHandler.js	74.35%	30 Missing ⚠️
lib/core/diagnostics.js	84.74%	9 Missing ⚠️
lib/api/readable.js	88.88%	5 Missing ⚠️
lib/eventsource/eventsource.js	96.09%	5 Missing ⚠️
lib/fetch/headers.js	90.19%	5 Missing ⚠️
lib/client.js	95.23%	3 Missing ⚠️
lib/compat/dispatcher-weakref.js	57.14%	3 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2608      +/-   ##
==========================================
- Coverage   85.54%   85.44%   -0.11%     
==========================================
  Files          76       84       +8     
  Lines        6858     7544     +686     
==========================================
+ Hits         5867     6446     +579     
- Misses        991     1098     +107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KhafraDev]

private properties are fine and preferred :)

regarding the WPTs we have our own test runner, but it's not as easy to use (you'd have to implement the routes for yourself, for example). There is also a subsequent section of the contributing guide that may help. I'd be willing to do this for you if you'd prefer, it'd save you a lot of work 😅.

[Uzlopak]

@KhafraDev

I would be very grateful if you would add the wpt tests :). I invited you to my fork. So should be easier for you to directly pushing to it.

It feels strange, that some attributes are private and some are symbols. Also comparing to other parts of the codebase looks not very tidy.

Anyway: Looking forward for every nitpick.

[KhafraDev]

added the tests, you can run them by doing node test/wpt/start-eventsource.mjs

[KhafraDev]

I can start reviewing now if you'd like, but currently the WPTs are failing because the WPTs use relative urls but you're not passing a base URL, for example:

undici/lib/websocket/websocket.js

Lines 61 to 67 in d3d825e

	const baseURL = getGlobalOrigin()
	// 1. Let urlRecord be the result of applying the URL parser to url with baseURL.
	let urlRecord
	try {
	urlRecord = new URL(url, baseURL)

[Uzlopak]

@KhafraDev

Awesome! Thanks for the snippet with getGlobalOrigin. Now 3 only wpt tests fail, which are related to the EventSourceStream, which I already said is not up to standard.

Please review the code, so that I can tackle them asap you write that you finished the review :).

Please be nitpicky as possible :)

Thank you!

[Uzlopak]

I will continue tonight to work on this. It feels much cleaner to implement it in undici than in node core itself.

Stay tuned ;)

[metcoder95]

So far LGTM, nice work @Uzlopak 🚀
I'd like to go on a second review later this weekend, to also have the time to read the spec a bit more; if that's ok?

[Uzlopak]

I have following tasks to do:

• Make it more like the fetch implementation where every step is commented implemented according to the spec
• Make the parser feature complete
• Check if the state parameter, shared between the EventSource and EventSourceStream is complete, or if it should be modified
• 100% test coverage, if possible

But you are invited to review and nitpick as much as possible as early as possible ;)

[Uzlopak]

Ok, I figured now out how to it like you did it for fetch. Obviously I have to implement it differently. My local code is not working, so I wont push. Tomorrow/Sunday I willl take care of it.

[Uzlopak]

I think for a first implementation this is good enough. Maybe the Buffer.subarray calls can be reduced. But it is currently working. So would be happy if you do a review now ;).

[mcollina]

Can you add TODOs to remove subarrays and concats for performance reasons?

[KhafraDev]

@Uzlopak we already drop those headers on cross-origin redirect

[Uzlopak]

@KhafraDev
perfect!

@mcollina
Added the TODOs and created an issue with some more information regarding it. Maybe #2630 can be then labelled good-first-issue?

[mcollina]

I don't think it'd be so easy ;).

[mcollina]

I'd prefer to wait until the new v21.6.1 release is out so we can test this there too.

[metcoder95]

Nice work 🚀

[Uzlopak]

@mcollina
@KhafraDev
@metcoder95
The CI/CD pipeline is green now ;).