Use buffer-alloc-unsafe to create emptyBuffer
#1154
Conversation
|
Thanks! We need this, however: "preferably Buffer.from(), Buffer.allocUnsafe() or Buffer.alloc()" So please, use one of those :) Not |
|
@joskuijpers I saw from #1017 (comment) that using
So I am not sure what is the compatibility plan for the community. Personally I am more than glad to use |
|
Oh stupid, of course! You are right, I wasn't thinking :) @brianc needs to decide what the changes are in backwards compatibility. We might make a new major update not node-backwards compatible. |
|
I would recommend using the |
|
buffer-alloc-unsafe looks good - that way we can keep backwards compatibility! As an aside - any idea why the tests are failing in every environment on travis? AFAIK nothing has changed on this end in a long time. I can't merge this until those tests are passing. I don't have time to look at it for the next few days, but might have time next week. |
|
@brianc As of 2016-10-31, nodejs is to discontinue support on v0.10, shall we still need support v0.10? @LinusU 👍 for |
JLHwung
changed the title
new Buffer() to create emptyBufferbuffer-alloc-unsafe to create emptyBuffer
@brianc The tests are failing because pg doesn’t specify exact dependencies and pg-pool 1.5.0 is broken. The brokenness is due to an incorrect fix for brianc/node-pg-pool#24 in the form of brianc/node-pg-pool#28. The bug (along with a few others) is fixed properly but backwards-incompatibly in brianc/node-pg-pool#31. |
|
I hope this will be merged soon. |
| //http://developer.postgresql.org/pgdocs/postgres/protocol-message-formats.html | ||
|
|
||
| var buffers = {}; | ||
| buffers.readyForQuery = function() { | ||
| return new BufferList() | ||
| .add(Buffer('I')) | ||
| .add(allocUnsafe('I')) |
There was a problem hiding this comment.
This should probably use Buffer.from(), ponyfill here. Buffer.allocUnsafe is just for allocating a new buffer with a specific length
| @@ -23,7 +24,7 @@ buffers.authenticationCleartextPassword = function() { | |||
| buffers.authenticationMD5Password = function() { | |||
| return new BufferList() | |||
| .addInt32(5) | |||
| .add(Buffer([1,2,3,4])) | |||
| .add(allocUnsafe([1, 2, 3, 4])) | |||
| @@ -71,7 +72,7 @@ buffers.dataRow = function(columns) { | |||
| if(col == null) { | |||
| buf.addInt32(-1); | |||
| } else { | |||
| var strBuf = new Buffer(col, 'utf8'); | |||
| var strBuf = allocUnsafe(col, 'utf8'); | |||
| @@ -94,7 +95,7 @@ var errorOrNotice = function(fields) { | |||
| buf.addChar(field.type); | |||
| buf.addCString(field.value); | |||
| }); | |||
| return buf.add(Buffer([0]));//terminator | |||
| return buf.add(allocUnsafe([0]));//terminator | |||
|
|
||
| test('md5 authentication', function() { | ||
| var client = createClient(); | ||
| client.password = "!"; | ||
| var salt = Buffer([1, 2, 3, 4]); | ||
| var salt = allocUnsafe([1, 2, 3, 4]); |
| @@ -159,7 +161,7 @@ test('Connection', function() { | |||
| testForMessage(plainPasswordBuffer, expectedPlainPasswordMessage); | |||
| var msg = testForMessage(md5PasswordBuffer, expectedMD5PasswordMessage); | |||
| test('md5 has right salt', function() { | |||
| assert.equalBuffers(msg.salt, Buffer([1,2,3,4])); | |||
| assert.equalBuffers(msg.salt, allocUnsafe([1, 2, 3, 4])); | |||
| @@ -174,7 +176,7 @@ test('Connection', function() { | |||
| }); | |||
|
|
|||
| test("no data message", function() { | |||
| testForMessage(Buffer([0x6e, 0, 0, 0, 4]), { | |||
| testForMessage(allocUnsafe([0x6e, 0, 0, 0, 4]), { | |||
| @@ -104,12 +106,12 @@ test('bind messages', function() { | |||
| .addInt16(0) | |||
| .addInt16(4) | |||
| .addInt32(1) | |||
| .add(Buffer("1")) | |||
| .add(allocUnsafe("1")) | |||
| .addInt32(2) | ||
| .add(Buffer("hi")) | ||
| .add(allocUnsafe("hi")) |
| .addInt32(-1) | ||
| .addInt32(4) | ||
| .add(Buffer('zing')) | ||
| .add(allocUnsafe('zing')) |
| @@ -120,7 +122,7 @@ test('with named statement, portal, and buffer value', function() { | |||
| con.bind({ | |||
| portal: 'bang', | |||
| statement: 'woo', | |||
| values: ['1', 'hi', null, new Buffer('zing', 'UTF-8')] | |||
| values: ['1', 'hi', null, allocUnsafe('zing', 'UTF-8')] | |||
| @@ -132,12 +134,12 @@ test('with named statement, portal, and buffer value', function() { | |||
| .addInt16(1)//binary | |||
| .addInt16(4) | |||
| .addInt32(1) | |||
| .add(Buffer("1")) | |||
| .add(allocUnsafe("1")) | |||
| .addInt32(2) | ||
| .add(Buffer("hi")) | ||
| .add(allocUnsafe("hi")) |
| .addInt32(-1) | ||
| .addInt32(4) | ||
| .add(new Buffer('zing', 'UTF-8')) | ||
| .add(allocUnsafe('zing', 'UTF-8')) |
| @@ -181,7 +183,7 @@ test('sends sync command', function() { | |||
|
|
|||
| test('sends end command', function() { | |||
| con.end(); | |||
| var expected = new Buffer([0x58, 0, 0, 0, 4]); | |||
| var expected = allocUnsafe([0x58, 0, 0, 0, 4]); | |||
| @@ -50,7 +50,7 @@ test('normalizing query configs', function() { | |||
| }) | |||
|
|
|||
| test('prepareValues: buffer prepared properly', function() { | |||
| var buf = new Buffer("quack"); | |||
| var buf = allocUnsafe("quack"); | |||
| @@ -142,7 +142,7 @@ test('prepareValue: objects with simple toPostgres prepared properly', function( | |||
| }); | |||
|
|
|||
| test('prepareValue: objects with complex toPostgres prepared properly', function() { | |||
| var buf = new Buffer("zomgcustom!"); | |||
| var buf = allocUnsafe("zomgcustom!"); | |||
| @@ -165,7 +165,7 @@ test('prepareValue: objects with toPostgres receive prepareValue', function() { | |||
| }); | |||
|
|
|||
| test('prepareValue: objects with circular toPostgres rejected', function() { | |||
| var buf = new Buffer("zomgcustom!"); | |||
| var buf = allocUnsafe("zomgcustom!"); | |||
With any luck, it will be possible to drop Node 0.10–0.12 support by then. In the meantime, these “ponyfills” add no benefit, and the suggested ones break the rules (in addition to the problems previously noted):
|
|
I'm confused. I'd say they add one clear benefit @charmander. They use the non-deprecated implementation whenever possible. Using deprecated methods is almost by definition not the right way to do something wouldn't you say? I'll have to have a more thorough think / read on the ponyfill situation. From my understanding both safe-buffer and @LinusU's buffer modules are ponyfills. |
|
@charmander after reading up more I think the point Sindre is trying to make is that ponyfills could potentially work differently in different environments which would be a very bad thing and I agree. In this case both ponyfills I linked in my elaboration use the native api in both cases, which if I understand correctly the node team makes sure result in identical functionality. In this case using the available native api in both cases is using the same implementation and thus doesn't break Sindre's rule. |
On mobile now, but I'm quite sure that that is necessary in this case since if it only relied on the |
|
As node.js has reverted buffer runtime deprecation in nodejs/node#9529, I think this pull request can be closed. |
|
@JLHwung how so? They still want to move forward with the deprecation. The current implementation this library uses is still marked deprecated and eventually will face harsher deprecation it seems. The core team seems unambiguous in their intention for users to use the new API, just unsure on how to help the ecosystem get there. This PR gets us there, with backwards compatibility and next to zero added dependency. Let's help make this easier on the core team ^_^. |
$ npm info buffer-from dependencies
{ 'is-array-buffer-x': '^1.0.13' }
$ npm info is-array-buffer-x dependencies
{ 'is-object-like-x': '^1.0.11',
'to-string-tag-x': '^1.0.11',
'has-to-string-tag-x': '^1.0.10' }
$ npm info is-object-like-x dependencies
{ 'is-function-x': '^1.0.6', 'is-primitive': '^2.0.0' }
$ npm info is-function-x dependencies
{ 'has-to-string-tag-x': '^1.0.10',
'to-string-tag-x': '^1.0.11',
'is-primitive': '^2.0.0' }
$ npm info has-to-string-tag-x dependencies
{ 'has-symbol-support-x': '^1.0.11' }
$ npm info to-string-tag-x dependencies
{ 'lodash.isnull': '^3.0.0', 'validate.io-undefined': '^1.0.3' }
$ npm info buffer-alloc dependencies
{ 'buffer-alloc-unsafe': '^0.1.0', 'buffer-fill': '^0.1.0' }If you’re dead set on using var bufferAlloc = Buffer.alloc || Buffer;
var bufferFrom = Buffer.from || Buffer; |
|
@charmander I don't see why you would look at the full dependency tree unless you're going to argue the library we're suggesting to take on is far too abstract and over complicates. I don't want to argue the details. You are right, it does add a number of dependencies. I'm trying not to be dead set on anything. I do think merging this is better for you, me and the ecosystem :]. Also, your code suggestion looks fine to me. Beyond a small change I'd make it's just style differences I think. Either abstract away the complexity of using a new API with backward compatibility or kill the dependency and own it. Up to all the people involved I'd say. I would like to add that the friendly cat you linked earlier would probably be in favor of the ponyfills, based on his writing about small dependencies. Thanks for helping us move with the node team Charmander! |
I think the library is well-written! My concern, though, is all these dependencies with caret-versions. We’ve just seen pg-pool 1.5 unintentionally break the build – maintainers can’t be perfect all the time. This is a set of 11 new packages, any one of which could have a new patch version published at some point in the future that affects behaviour in a surprising way. So, nothing against small packages*, but in the interest of reducing points of failure I’m for eating the maintenance costs in pg itself in the end, given only those two options. Requiring a supported version of Node with * Except for |
|
@charmander cool! I can only agree. I would prefer to drop node < 4. I guess now we wait for someone with merge powers to show up and start moving! |
|
I don't think that the problem with caret should be solved by dropping them. There are solutions to this problem baked right into npm; npm shrinkwrap. Another popular solution is Yarn |
|
All the way there with you @LinusU! I think perhaps it'd be sensible to wait for a maintainer to voice how they even feel in the first place about the PR as it stands now. |
|
Hey y'all - I really appreciate you going through this. Sorry I've been out for a while. I've been maintaining node-postgres for >6 years most of the time solo, and always for free, and sometimes life gets really hectic, and I have to step away for some time. I'm finding a bit more time now, things are calming down, so that's good. Just wanted to give a bit of background as to why this has sit for so long. I'm going to do a better job of finding more maintainers here so I'm not a single point of failure anymore. As far as this PR is concerned, @charmander did the lovely work of getting CI passing again 🙏 . I would still like to support What I'd like to see: can we get this rebased back with the current master so we pick up the fixes for travis? Once the CI is running I can get a good look and see what versions of node are broken. If as an aside: it's annoying that node changed the api for tl;dr - rebase this with master so we can get it running in Travis. After that I'll merge this. |
|
@brianc Rebased on master. Appreciated for your attitude. |
|
This PR is no longer needed. The depreciation warning has been removed from Node.js, starting from version 7.2.1 See nodejs/node#9529 |
|
Awesome to see your face @brianc! Glad you found the time. Totally understandable it took a bit! @vitaly-t see this comment a little bit up. I replied to it already. |
Using Buffer without `new` will soon stop working from node v7.0.0. Use `new Buffer()`, or preferably `Buffer.from()`, `Buffer.allocUnsafe()` or `Buffer.alloc()` instead.
… except in dependencies.
|
@brianc Rebased on master. |
|
pg@7 is going to be able to use |
Deprecation Warning when testing against node 7.0.0 rc1
I know we shall support node prior to 6.x, so I use
new Buffer()to replaceBuffer().