Add WS dep to emscripten_websocket_new (fixes #26310)#26317
Add WS dep to emscripten_websocket_new (fixes #26310)#26317floooh wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a missing dependency in the WebSocket JS library so emscripten_websocket_new correctly pulls in the handle allocator needed to create the webSockets global (addressing #26310).
Changes:
- Add an explicit dependency for
emscripten_websocket_newto ensure required websocket runtime globals are generated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return {{{ cDefs.EMSCRIPTEN_RESULT_SUCCESS }}}; | ||
| }, | ||
|
|
||
| emscripten_websocket_new__deps: ['$WS'], |
There was a problem hiding this comment.
emscripten_websocket_new only references the webSockets handle allocator (it doesn't use the WS helper object). Depending on $WS will also pull in malloc via $WS__deps, which can unnecessarily increase code size for minimal builds that only call emscripten_websocket_new. Consider changing this dependency to $webSockets (or otherwise avoid bringing in malloc) so the fix is minimal and directly matches what the function uses.
| emscripten_websocket_new__deps: ['$WS'], | |
| emscripten_websocket_new__deps: ['$webSockets'], |
| return {{{ cDefs.EMSCRIPTEN_RESULT_SUCCESS }}}; | ||
| }, | ||
|
|
||
| emscripten_websocket_new__deps: ['$WS'], |
There was a problem hiding this comment.
This fixes a missing dependency edge case (calling emscripten_websocket_new without any other websocket APIs). To prevent regressions, please consider adding a focused test that only links/calls emscripten_websocket_new (e.g., compile a tiny C program with -lwebsocket and run it) and asserts it doesn't throw ReferenceError: webSockets is not defined.
| emscripten_websocket_new__deps: ['$WS'], | |
| emscripten_websocket_new__deps: ['$WS', '$webSockets'], |
|
...does that Copilot suggestion even make sense? Please advice ;) |
|
Yes, I actually think both those copolit suggestions are good .. I guess you are paying gopilot since I don't think I've seen it comment on emscripten PRs before. |
|
Hmm, weird, Copilot isn't active on my own projects and I don't remember haven't activated Copilot reviews or similar... But anyway... I think the other PR at #26318 implements the same fix in a better way. Feel free to close my PR and merge the other one :) |
|
I think the tricky part is likely adding a test, so whichever one of you if up for that we can land. |
|
Closing in favor of #26318 i |
2 participants
This is a single-line fix to add a missing
$WSdependency to theemscripten_websocket_newfunction.See #26310 for details.
Not really a high-priority fix, since I guess
emscripten_websocket_newis rarely used alone, and the otheremscripten_websocket_*functions will pull in the same dependency anyway (for the same reason I think it's not worth to add a new test for this specific problem - I did test the fix locally though with the code in the issue).