search

shovon / node-rudp

Reliable UDP for Node.js
33 stars 14 forks source link

replace buffertools with buffer-equal #3

Closed mvayngrib closed 9 years ago

mvayngrib commented 9 years ago

node-rudp only uses the buffer comparison method, so this is a smaller dep. Also buffertools is not browserifiable.

shovon commented 9 years ago

Sorry, I should have been a bit more diligent when hitting the merge button, but I was on my phone and I initially thought that the pull request was innocuous. But it turns out, it's a bit more complicated than that. buffer-equal uses an O(n) algorithm, written in JavaScript, where as buffertools uses memcmp.

I'm not too sure what are the performance implications of moving from buffertools to buffer-equal. I know, I know! Saying that "C++ is faster than JavaScript" is naive, but I would rather err on the side of caution.

@mvayngrib if you can, please show some benefits to your pull request, then I will be happy to merge it in.

And I might be wrong, but I don't think JavaScript on the browser has any means to interface with the transport layer. That is, I don't think, with the given JavaScript APIs, you can send UDP packets to a host, without an ugly hack. Ultimately, I don't think "browserifiability" is a benefit that I can factor in.

mvayngrib commented 9 years ago

oh, interesting. I'm actually using this not in the browser, but in react-native, which also uses browser replacement mappings. Maybe let's instead put a mapping for "browser" in package.json to use "buffer-equal" in the "browser" - whether that "browser" is actually a browser or a chrome app or a native app. If that sounds like a good compromise to you, I'll adjust the PR

shovon commented 9 years ago

I'm actually using this not in the browser, but in react-native

Oh, didn't realize that that's what you were using node-rudp for.

Maybe let's instead put a mapping for "browser" in package.json to use "buffer-equal" in the "browser" - whether that "browser" is actually a browser or a chrome app or a native app. If that sounds like a good compromise to you, I'll adjust the PR

I will be all for it.

mvayngrib commented 9 years ago

i created a separate one, don't know if you can merge this one again: https://github.com/shovon/node-rudp/pull/5