Switch to bufferjs from buffertools. #36

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

domenic commented Jun 29, 2012

Buffertools doesn't work on Windows ever since 0.8.0 came out, due to TooTallNate/node-gyp#90. Moving to a JS module instead of a native one seems like a big win, anyway.

If you are concerned about the speed implications, we could maybe make buffertools an optionalDependency and fall back to bufferjs. But I doubt benchmarks would show much difference?

Owner

eleith commented Jun 29, 2012

no, i'm totally happy to switch to bufferjs. it would resolve the majority of issues i've seen with people trying to use emailjs. it is worth what likely amounts to a tiny and likely unobservable-by-most decrease in performance.

i'll try it out this weekend and accept the patch and update npm if all works well.

Contributor

mscdex commented Jun 29, 2012

IMHO mucking with core objects (e.g. Buffer), which bufferjs does, is generally a bad thing. I've learned this first hand.

EDIT: Also, bufferjs's concat() implementation is not as efficient as it could be.

@mscdex mscdex commented on an outdated diff Jun 29, 2012

smtp/message.js
@@ -343,6 +344,7 @@ var MessageStream = function(message)
// do we have bytes from a previous stream data event?
if(previous)
{
+ var buffer2 = Buffer.concat(previous, buffer);
@mscdex

mscdex Jun 29, 2012

Contributor

I think you meant to modify the line below instead of just adding this duplicate line?

Owner

eleith commented Jun 29, 2012

the problem seems to be buffertools and windows are not working well because of the newly introduced gyp.

has any one gotten buffertools working on node 0.8 on windows? ...

Contributor

mscdex commented Jun 29, 2012

FWIW the latest buffertools (1.1.0 as of this writing) built fine for me on Windows XP64 with node 0.8.1 and npm 1.1.33.

Contributor

domenic commented Jun 29, 2012

Ugh I did indeed forget to delete the old line, sorry. Fixed and force-pushed.

Owner

eleith commented Jul 2, 2012

Buffer.concat is now a method in 0.8.1!

would you recommend making it an optional dependency or is their a better way to match up older versions of node with buffertools and newer versions to just use Buffer.concat ?

eleith closed this Jul 2, 2012

Contributor

mscdex commented Jul 2, 2012

I'm not really sure of the best way off the top of my head. One way would be to have a preinstall script that checks the node version and then if it's < 0.7.11 (first release containing Buffer.concat), start a child process that executes "npm install buffertools".

Owner

eleith commented Jul 2, 2012

i have bufferjs as an optionalDependency in package.json, so older versions of node will pick up a less than ideal solution to Buffer.concat, but newer versions will benefit from the native implementation.

since concat only happens when attaching streams, this seems to be the safest path that encourages being on the latest stable nodejs.

Contributor

mscdex commented Jul 2, 2012

Well, all optionalDependencies means is that after trying to install the dependency, don't abort if the dependency installation failed. So, even for node 0.8.x users, requiring bufferjs at the top of the script will actually overwrite the core implementation. There's probably not too much of a difference between bufferjs's concat and node core's concat, since both are implemented in js-land.

Owner

eleith commented Jul 2, 2012

i only include bufferjs when Buffer.concat is missing. you are right, their implementations are pretty close.

Contributor

domenic commented Jul 2, 2012

Maybe publish a version with a (non-optional) bufferjs dependency, so 0.6 users can get their fix, then remove it, set the min version to 0.7.whatever, and publish a new version? In theory this might lead to maintaining parallel branches for as long as you want to support 0.6, but in practice this package is pretty stable, so…

On Jul 2, 2012, at 0:21, "Brian White" reply@reply.github.com wrote:

Well, all optionalDependencies means is that after trying to install the dependency, don't abort if the dependency installation failed. So, even for node 0.8.x users, requiring bufferjs at the top of the script will actually overwrite the core implementation. There's probably not too much of a difference between bufferjs's concat and node core's concat, since both are implemented in js-land.


Reply to this email directly or view it on GitHub:
#36 (comment)

GammaNu referenced this pull request in dodo/node-bufferstream Jun 18, 2013

Closed

can't use it on windows cause buffertools dependency #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment