-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
feat: implement EventSource #51421
feat: implement EventSource #51421
Conversation
Review requested:
|
My 2 cents is that this is better implemented as part of Undici. |
I would be glad, if we integrate this first into undici and then expose it to node core via integrated undici. |
I have the problem, that NodeEventTarget is not exposed. I can get kEvents Symbol indirectly. But NodeEventTarget is not possible to load from userland. I could duplicate the code of NodeEventTarget for an undici implementation, but this doesnt seem right. |
You can get it with |
EventTarget is missing e.g. removeAllListeners, which is quite handy to manage the kEvents of EventTarget can be retrieved by doing const kEvents = Object.getOwnPropertySymbols(new EventTarget())
.find((symbol) => {
return symbol.description === 'kEvents'
}) But I guess, on second thought, I can do it with kEvents to access the Map. But it is not that nice tbh. |
Ok for it to stay here. |
This is better in undici, I agree with @mcollina. We already implement MessageEvent and obviously fetch. Managing the events is quite easy and we do it without NodeEventTarget. |
I will have a look at it tonight (well it is already night) ;). I actually like it also to be in undici, as it opens my hands for a better structuring of the files instead of putting everything into a single event_stream.js. ;). I will keep you updated. |
I'm willing to help if you have any questions. |
Closing in favor of nodejs/undici#2608 See you over there :D |
This PR is not author-ready.
I wanted to propose to add EventSource as native module. This is working in a reasonable way and I personally like the overall structure of the implementation.
Still has to be made full spec compliant and hardened. Also needs the use of primordials and the EventSourceStream-class is not handling EOL characters properly, which needs to be implemented properly.
But before I invest more, I wanted to discuss two aspects of this implementation, which need clarification as they popped up while implementing.
Anyhow, I worked on it for few days an now I want to clarify with you, if this feature is even desired. Not that I finish the nicest EventSource implementation, but it has no chance to get integrated into node core.
Looking forward to your feedback regarding my questions. If there is positive feedback then I am glad to finish the code ;).