hildjj / node-cbor

Encode and decode CBOR documents, with both easy mode, streaming mode, and SAX-style evented mode.
MIT License
356 stars 73 forks source link

Fix Buffer converter matcher being mangled in minimised builds #166

Closed noway closed 2 years ago

noway commented 2 years ago

Fixes #164

Happy to get this up to production quality/fix some other parts if i'm missing something.

hildjj commented 2 years ago

Does this work often enough to be worth it? isBuffer just uses instanceof, and if the two Buffer implementations are different, instanceof won't return true.

hildjj commented 2 years ago

I'd be wiling to add exports.Buffer = require('buffer').Buffer to cbor.js, which would allow everyone to use the same (already minified) Buffer implementation.

noway commented 2 years ago

Hi @hildjj, thank you for taking the time to review this and also maintaining the library!

isBuffer just uses instanceof, and if the two Buffer implementations are different, instanceof won't return true.

I'm not sure I fully get this argument, I might be missing something.

Buffer implementation is part of the javascript bundle I vendor: https://unpkg.com/@vaxxnz/nzcp@0.1.5/dist/esbuild/browser.js.

I don't expose any Buffer interfaces, so it's guaranteed I always originate the usage of Buffer inside the library. While Buffer.name gets mangled and can differ, Buffer instance itself is the same inside the bundle.

If I were to expose some Buffer interfaces, I could run Buffer.from() on them to make sure I'm using the right Buffer implementation. It's not ideal, but workable.

What's a scenario when this would break down?

Cheers.

noway commented 2 years ago

The workaround from https://github.com/hildjj/node-cbor/issues/164#issuecomment-994020193 solves this issue for me, so I going to close this pull request.