Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 10, 2024

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.

  1. 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
  2. Do we actually want to integrate EventSource to nodejs? deno implemented it, but should we do it too?

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 ;).

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup
  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. tools Issues and PRs related to the tools directory. labels Jan 10, 2024
@mcollina
Copy link
Member

My 2 cents is that this is better implemented as part of Undici.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 10, 2024

@mcollina

I would be glad, if we integrate this first into undici and then expose it to node core via integrated undici.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 10, 2024

@mcollina

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.

@targos
Copy link
Member

targos commented Jan 10, 2024

You can get it with Object.getPrototypeOf(MessagePort) for example, but why do you need it? Shouldn't EventSource extend from EventTarget directly? We don't need the compatibility layer of NodeEventTarget in new APIs unless I'm missing something.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 10, 2024

EventTarget is missing e.g. removeAllListeners, which is quite handy to manage the onmessage, onerror and onopen attributes.

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.

@mcollina
Copy link
Member

Ok for it to stay here.

@KhafraDev
Copy link
Member

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.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 10, 2024

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.

@KhafraDev
Copy link
Member

I'm willing to help if you have any questions.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 11, 2024

Closing in favor of nodejs/undici#2608

See you over there :D

@Uzlopak Uzlopak closed this Jan 11, 2024
@Uzlopak Uzlopak deleted the eventsource-wip branch January 16, 2024 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants