Merge for #72 and #73 (instanceof, bl-without-streams) #74

rvagg · 2019-09-17T03:10:28Z

This is a merge of #72 and #73 with some additional pieces.

First 3 commits are a straight cherry-pick of #72, plus a fixed #73 to fit on top of it.

I had some difficulty getting #73's isBufferList() approach to play nicely with the tests, mainly because of the replacement of the if (!(this instanceof Type)) return new Type() construct at the top of the constructor(s). But, the only reason this construct is there is so that you can avoid new, so using instanceof shouldn't be a problem here even in the browser since you're not doing any mush-things-together operations, you're creating entirely new things. So I've restored that and I think the approach is fine even with the split BufferListStream and BufferList.

While I was playing with this I ended up bailing on the approach that both approaches to determining "isBufferList" and went with a Symbol set on a BufferList instance and checking that. Which gets us closer to instanceof I think. But it does make the duck-typing of #72 go away entirely, I'm not sure how you feel about that @jacobheun?

So @jacobheun and @alanshaw would you mind having a look over this?

And @mcollina if you have time for a review, even a quick one, that would be really helpful.

Further comments inline.

fix: dont use instanceof #71

Now you can get a `BufferList` without the `readable-stream` bits for a smaller browser bundle. `bl` now exports 2 classes: 1. `BufferList` 2. `BufferListStream` `BufferListStream` **is the default export** (and so is backwards compatible). i.e. ```js const { BufferListStream } = require('bl') // equivalent to: const BufferListStream = require('bl') ``` I've updated the README to recommend typical use like: ```js const { BufferList } = require('bl') // or const { BufferListStream } = require('bl') ``` Simply `require('bl/BufferList')` in the browser to get **JUST** the `BufferList` class that does not inherit from `Duplex`. Added static function `BufferList.isBufferList(obj)` which checks if the passed object has a `_isBufferList` property (similar to how `Buffer.isBuffer(obj)` works). resolves #48 resolves #70 resolves #71

rvagg · 2019-09-17T03:10:57Z

BufferList.js

+  BufferList._init.call(this, buf)
+}
+
+BufferList._init = function _init (buf) {


had to resort to this to avoid circular calls due to the two constructors getting tangled, unfortunately

rvagg · 2019-09-17T03:12:52Z

README.md

@@ -12,9 +12,9 @@
 The original buffers are kept intact and copies are only done as necessary. Any reads that require the use of a single original buffer will return a slice of that buffer only (which references the same memory as the original buffer). Reads that span buffers perform concatenation as required and return the results transparently.

 ```js
-const BufferList = require('bl')
+const { BufferList } = require('bl')


I can live with it but I'm not entirely comfortable making the old style completely disappear from the README. This is only 4 more characters but it's not clear from the README that the old style is still possible. Thoughts from anyone else?

Worth mentioning it is documented later that BufferListStream is the default export (so you can continue to use it as before) and the way we've done this won't break existing users...unless there's some unforseen issue!

It's also worth pointing out that BufferListStream is the equivalent of what users currently get when they require('bl') and that the documentation has changed to effectively recommend using the non-streamy version (first example is using BufferList not BufferListStream).

I half assumed this would be a major release but it's completely backwards compatible so it doesn't have to be. We could tone down the change in the README to not look so "major" if you were planning on releasing as a minor and it is a concern.

IMHO as a major release this documentation change is absolutely fine, as a minor it's probably still fine, but it has the potential to confuse some existing users (although not actually break their code).

rvagg · 2019-09-17T03:14:56Z

test/convert.js

+  const data = Buffer.from(`TEST-${Date.now()}`)
+  const bl = new BufferList(data)
+  const bls = new BufferListStream(bl)
+  t.ok(bl.slice().equals(bls.slice()))


These two were t.equal(bl.slice(), bls.slice()) in #73, strict equality rather than buffer equality. These don't work because #72 unwraps the Buffer instances and rebuilds a new one from the underlying ArrayBuffer. Does this matter to anyone?

Considering the documentation has always said that slice returns a new Buffer I don't think there should be any expectations that it returns the same buffer. I think the test was wrong.

removes duck-typing ability, restores non-constructor-as-constructor use Closes: #72 Closes: #73

alanshaw · 2019-09-17T10:13:58Z

README.md

@@ -12,9 +12,9 @@
 The original buffers are kept intact and copies are only done as necessary. Any reads that require the use of a single original buffer will return a slice of that buffer only (which references the same memory as the original buffer). Reads that span buffers perform concatenation as required and return the results transparently.

 ```js
-const BufferList = require('bl')
+const { BufferList } = require('bl')


Worth mentioning it is documented later that BufferListStream is the default export (so you can continue to use it as before) and the way we've done this won't break existing users...unless there's some unforseen issue!

It's also worth pointing out that BufferListStream is the equivalent of what users currently get when they require('bl') and that the documentation has changed to effectively recommend using the non-streamy version (first example is using BufferList not BufferListStream).

I half assumed this would be a major release but it's completely backwards compatible so it doesn't have to be. We could tone down the change in the README to not look so "major" if you were planning on releasing as a minor and it is a concern.

IMHO as a major release this documentation change is absolutely fine, as a minor it's probably still fine, but it has the potential to confuse some existing users (although not actually break their code).

alanshaw · 2019-09-17T10:35:57Z

test/convert.js

+  const data = Buffer.from(`TEST-${Date.now()}`)
+  const bl = new BufferList(data)
+  const bls = new BufferListStream(bl)
+  t.ok(bl.slice().equals(bls.slice()))


Considering the documentation has always said that slice returns a new Buffer I don't think there should be any expectations that it returns the same buffer. I think the test was wrong.

jacobheun

But it does make the duck-typing of #72 go away entirely, I'm not sure how you feel about that @jacobheun?

The primary reason I used duck-typing was for supporting verification of older versions of bl. I think the change to symbols is fine. Transitive dependencies just need to get updated and worst case we can back port the change.

rvagg · 2019-09-18T03:35:06Z

I think we'll just do a semver-major then eh? Will give @mcollina a day or two more but I'm going to follow up with another PR to adopt standard and fix some of the older syntax (var being the big one).

rvagg · 2019-09-18T04:12:46Z

My current proposal is to do this PR and #75 for a 4.0.0 release. I know it's technically not breaking but I do prefer caution when it comes to semver. If bl is working for folks then no need to upgrade, if you want the extra nice bits introduced here then grab 4.0.0.

rvagg · 2019-09-19T02:14:07Z

out with 4.0.0, thanks @alanshaw & @jacobheun

now we just need that .equals() suggested in #68

jacobheun and others added 3 commits September 17, 2019 11:59

feat: add support for Uint8Array

71924e9

fix: dont use instanceof #71

test: add _bufs checks for append and indexOf

9b7f6c4

rvagg commented Sep 17, 2019

View reviewed changes

merge isBufferList & duck-typing approaches using symbol

c053c7d

removes duck-typing ability, restores non-constructor-as-constructor use Closes: #72 Closes: #73

rvagg force-pushed the rvagg/instanceof-merge branch from b425651 to c053c7d Compare September 17, 2019 03:18

alanshaw approved these changes Sep 17, 2019

View reviewed changes

rvagg changed the title ~~Merge or #72 and #73 (instanceof, bl-without-streams)~~ Merge for #72 and #73 (instanceof, bl-without-streams) Sep 17, 2019

jacobheun approved these changes Sep 17, 2019

View reviewed changes

rvagg mentioned this pull request Sep 18, 2019

style: use 'standard', minor formatting & syntax changes #75

Merged

rvagg merged commit 834ba70 into master Sep 19, 2019

rvagg deleted the rvagg/instanceof-merge branch September 19, 2019 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge for #72 and #73 (instanceof, bl-without-streams) #74

Merge for #72 and #73 (instanceof, bl-without-streams) #74

rvagg commented Sep 17, 2019

rvagg Sep 17, 2019

rvagg Sep 17, 2019

alanshaw Sep 17, 2019

rvagg Sep 17, 2019

alanshaw Sep 17, 2019

alanshaw Sep 17, 2019

alanshaw Sep 17, 2019

jacobheun left a comment

rvagg commented Sep 18, 2019

rvagg commented Sep 18, 2019

rvagg commented Sep 19, 2019

Merge for #72 and #73 (instanceof, bl-without-streams) #74

Merge for #72 and #73 (instanceof, bl-without-streams) #74

Conversation

rvagg commented Sep 17, 2019

rvagg Sep 17, 2019

Choose a reason for hiding this comment

rvagg Sep 17, 2019

Choose a reason for hiding this comment

alanshaw Sep 17, 2019

Choose a reason for hiding this comment

rvagg Sep 17, 2019

Choose a reason for hiding this comment

alanshaw Sep 17, 2019

Choose a reason for hiding this comment

alanshaw Sep 17, 2019

Choose a reason for hiding this comment

alanshaw Sep 17, 2019

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

rvagg commented Sep 18, 2019

rvagg commented Sep 18, 2019

rvagg commented Sep 19, 2019