Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undo instanceof Buffer changes #155

Merged
merged 1 commit into from
Feb 9, 2017
Merged

Undo instanceof Buffer changes #155

merged 1 commit into from
Feb 9, 2017

Conversation

feross
Copy link
Owner

@feross feross commented Feb 9, 2017

It turns out that it's not possible to use instanceof Buffer safely, like I thought.

It's not possible to use instanceof Buffer reliably in a browserify context because there could be multiple different copies of the 'buffer' package in use.

This previous method (checking buf._isBuffer) works even for Buffer instances that were created from another copy of the buffer package.

NOTE: It's possible to have two different "instances" of the 'buffer' package, even if the 'buffer' package appears only once in the bundled code. This can happen if you require 'buffer' in different ways, for example:

require('buffer') vs. require('buffer/') vs. using the implicit Buffer global.

You can confirm this by browserifying this code:

console.log(require('buffer').Buffer === require('buffer/').Buffer) // will be false

So, for this reason, instanceof won't work.

Fixes: #154

It turns out that it's not possible to use `instanceof Buffer` safely,
like I thought.

It's not possible to use `instanceof Buffer` reliably in a browserify
context because there could be multiple different copies of the
'buffer' package in use.

This previous method (checking `buf._isBuffer`) works even for Buffer
instances that were created from another copy of the `buffer` package.

NOTE: It's possible to have two different "instances" of the 'buffer'
package, even if the 'buffer' package appears only once in the bundled
code. This can happen if you require 'buffer' in different ways, for
example:

`require('buffer')` vs. `require('buffer/')` vs. using the implicit
`Buffer` global.

You can confirm this by browserifying this code:

```js
console.log(require('buffer').Buffer === require('buffer/').Buffer) //
will be false
```

So, for this reason, `instanceof` won't work.
@feross feross requested a review from dcousens February 9, 2017 22:03
@feross feross merged commit 0d2666d into master Feb 9, 2017
@feross feross deleted the instanceof branch February 9, 2017 22:06
@feross
Copy link
Owner Author

feross commented Feb 9, 2017

Released as 5.0.5.

// reliably in a browserify context because there could be multiple different
// copies of the 'buffer' package in use. This method works even for Buffer
// instances that were created from another copy of the `buffer` package.
// See: https://github.com/feross/buffer/issues/154
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect

@dcousens
Copy link
Collaborator

dcousens commented Feb 9, 2017

LGTM, nice work @feross.
I did think of this as being an issue when I saw the instanceof, but didn't think anyone would realistically run into the issue since very few people explicitly require('buffer').
I guess I was wrong.

@feross
Copy link
Owner Author

feross commented Feb 9, 2017

Live and learn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants