-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
'_placeholder' in ESM module is replaced with 'it' which breaks binary data #5215
Comments
I could indeed reproduce the issue, I'm digging into this. |
The "_placeholder" attribute is used when sending binary data, and was incorrectly mangled (converted to a random short property, like "it", to reduce the bundle size). This bug was introduced in [1], included in `[email protected]`. [1]: 7085f0e Related: #5215
Thanks for the quick response. My workaround is to use a unit test to copy the file into the production folder and patch the two places. So it's not a killer at the moment. |
@digulla this should be fixed in version |
Thanks for the quick update! But it's now mangled to Can you add a unit test which loads the minified version and checks the output of the function? Or at least add a unit test which fails when the minified version doesn't contain the string Or if you want to do it manually, search for |
For reference, I'm using the Python unit test to fix the bug in 4.8.0:
|
Describe the bug
The file
socket.io.esm.min.js
from the NPM package socket.io-client 4.8.0 contains a bug which breaks binary data when talking to other SocketIO implementations (for example python-socjetio).Instead of
{_placeholder: true, num: 0}
, the code produces{it: true, num: 0}
. It's correct in the source. The problem is somewhere in the ESM build.To Reproduce
Search for
function _deconstructPacket
insocket.io.js
. It looks like this:In the ESM, it gets compiled into this code:
The same happens in
_reconstructPacket
. Source:which is compiled into
Expected behavior
The binary representation in the serialized message must be correct.
Platform:
Additional context
n.a.
The text was updated successfully, but these errors were encountered: