Several fixes to the node v0.6.x port #27

Merged
merged 1 commit into from Dec 13, 2011

2 participants

@ddopson
Collaborator

add setEncoding() to replace 'data_as_buffer'.
expose the public properties.
add 'connect' method that takes a callback.
fix zk_promise.

fix tests

Dave Dopson add setEncoding() to replace 'data_as_buffer'. expose the public prop…
…erties. add 'connect' method that takes a callback. fix zk_promise. fix tests
a947a95
@ddopson ddopson merged commit 8f18c8a into yfinkelstein:master Dec 13, 2011
@Woodya
Collaborator
@ddopson
Collaborator

I am in 100% agreement with you that the default behavior shouldn't change (principle of backwards compat).

However, I think in the original the default behavior was string. I'll go back and try to double-check that behavior. If you are right that the default behavior was Buffer, then 100% that should be the new default as well. In fact, that's the conventional default for Node, and thus my preference absent any backwards-compatible behavior.

"I did try to keep the module version in line with the zk lib version it worked with, using another variable for rev spin." -- that's a good idea that just occurred to me this morning. Fortunately, we haven't pushed the module to the NPM repo yet, so we can fix that. I had wanted to bump the version to indicate a major departure. Fortunately v3.3.x -> 3.4.x should be sufficient.

"to document the version it should work with " -- few clarifications: the wscript is written such that the module is statically linked. Thus it shouldn't depend on any local install of the zk library. I'm not a zk expert by any means, but hopefully you don't mean that there are any compat issues between the client lib and a down-version server? (dear god, I hope not, that would be aweful ugly). Either way, I agree with you on the versioning scheme and will make that change.

@ddopson
Collaborator

ok, so there's some confusion on my side.

Short Story: You are right.

Long Story: something about a test that failed in a way that made it seem like the default used to be string.

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