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

[WebSocket] Add CloseEvent #50275

Closed
regseb opened this issue Oct 19, 2023 · 8 comments · Fixed by #53355
Closed

[WebSocket] Add CloseEvent #50275

regseb opened this issue Oct 19, 2023 · 8 comments · Fixed by #53355
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. web-standards Issues and PRs related to Web APIs

Comments

@regseb
Copy link
Contributor

regseb commented Oct 19, 2023

What is the problem this feature will solve?

In Node.js v21 with --experimental-websocket, the CloseEvent class isn't defined. This event is sent to clients using WebSockets when the connection is closed.

What is the feature you are proposing to solve the problem?

Add the CloseEvent class in the global scope (behind --experimental-websocket flags). I found it in undici but it isn't exported. Or add the CloseEvent class next to MessageEvent (directly in Node.js).

What alternatives have you considered?

No response

Related issues

@regseb regseb added the feature request Issues that request new features to be added to Node.js. label Oct 19, 2023
Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 19, 2024
@regseb
Copy link
Contributor Author

regseb commented Apr 19, 2024

CloseEvent is part of the WebSockets specification. I think we need to keep this issue open if we want Node.js to be compliant with this specification.

@github-actions github-actions bot removed the stale label Apr 20, 2024
@RedYetiDev
Copy link
Member

@nodejs/undici

@RedYetiDev RedYetiDev added the web-standards Issues and PRs related to Web APIs label Apr 25, 2024
KhafraDev added a commit to KhafraDev/undici that referenced this issue Apr 26, 2024
Uzlopak pushed a commit to nodejs/undici that referenced this issue Apr 26, 2024
@mcollina mcollina added the good first issue Issues that are suitable for first-time contributors. label Apr 26, 2024
@mcollina
Copy link
Member

This is a actually easy to add.

@KhafraDev
Copy link
Member

once undici is updated it will be

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 26, 2024

Didnt I merge your PR @KhafraDev So it should be already part of the next release ;)

@thisis-akash
Copy link

thisis-akash commented Apr 30, 2024

I was digging around this issue in hopes that it could be my first open source contribution. I forked the repo and cloned it on my local. Haven't really gone further than that. The IDE configs and understanding the build process is a work in progress.

After spending a good amount of time, I am having an understanding that the issue is related to code in the dep: undici. The issue suggested to either fix it there or add that event directly in here. The fix is merged in the dep repo 5 days ago. Am I correct?

Is there anything still required here in this repo? I mean as per my understanding undici implements websocket and its defined on the globalThis and if CloseEvent is now exported from there (thanks to KhafraDev), then it should be available through globalThis.WebSocket. I can even see the CloseEvent
image

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 30, 2024

Undici has to release a new version. Then nodejs will update the dependency. And then you have to rebase your PR.

KhafraDev added a commit to KhafraDev/node that referenced this issue Jun 5, 2024
This PR adds `CloseEvent` as a global, which can be disabled via the --no-experimental-websocket flag.

```js
const ws = new WebSocket('...')

ws.addEventListener('close', (event) => {
  assert(event instanceof CloseEvent)
})

```

Fixes: nodejs#50275
KhafraDev added a commit to KhafraDev/node that referenced this issue Jun 5, 2024
This PR adds `CloseEvent` as a global, which can be disabled via the --no-experimental-websocket flag.

```js
const ws = new WebSocket('...')

ws.addEventListener('close', (event) => {
  assert(event instanceof CloseEvent)
})

```

Fixes: nodejs#50275
KhafraDev added a commit to KhafraDev/node that referenced this issue Jun 5, 2024
This PR adds `CloseEvent` as a global, which can be disabled
via the --no-experimental-websocket flag.

```js
const ws = new WebSocket('...')

ws.addEventListener('close', (event) => {
  assert(event instanceof CloseEvent)
})

```

Fixes: nodejs#50275
nodejs-github-bot pushed a commit that referenced this issue Jun 7, 2024
This PR adds `CloseEvent` as a global, which can be disabled
via the --no-experimental-websocket flag.

```js
const ws = new WebSocket('...')

ws.addEventListener('close', (event) => {
  assert(event instanceof CloseEvent)
})

```

Fixes: #50275
PR-URL: #53355
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. web-standards Issues and PRs related to Web APIs
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.

6 participants