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

Fix Buffer converter matcher being mangled in minimised builds #166

Closed
wants to merge 1 commit into from

Conversation

noway
Copy link

@noway noway commented Nov 12, 2021

Fixes #164

  • This fix might have performance implications - but it is passable for my use case.
  • There are other converter matchers which might be mangled - but I only fixed Buffer since that fixes my use case

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

@hildjj
Copy link
Owner

hildjj commented Nov 12, 2021

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
Copy link
Owner

hildjj commented Nov 12, 2021

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
Copy link
Author

noway commented Nov 16, 2021

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
Copy link
Author

noway commented Dec 19, 2021

The workaround from #164 (comment) solves this issue for me, so I going to close this pull request.

@noway noway closed this Dec 19, 2021
@noway noway deleted the fix/buffer-converter-matcher branch December 20, 2021 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webpack minimize causes cbor.encode to generate a different value
2 participants