Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Buffer shim is no longer working in the Browser as expected #651

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

joaosantos15
Copy link

@joaosantos15 joaosantos15 commented Dec 20, 2017

For #649
Adds a failing test for when adding an NPM Buffer:

let Buffer = require('buffer/').Buffer
let expectedBufferMultihash = 'QmWfVY9y3xjsixTgbd9AorQxH7VtMpzfx2HaWtsoUYecaX'
let file = Buffer.from('hello')

ipfs.files.add(file, ...

@joaosantos15
Copy link
Author

cc @diasdavid

Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Thanks @joaosantos15! Will this PR bring the fix as well?

@@ -62,6 +62,21 @@ describe('.files (the MFS API part)', function () {
})
})

it('file.add with NPM Buffer', (done) => {
let Buffer = require('buffer/').Buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the /?

Copy link
Author

@joaosantos15 joaosantos15 Dec 28, 2017

Choose a reason for hiding this comment

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

It's to depend on that module specifically. I tested without it and it was using Nodejs core Buffer module.
I read about the usage here:.

To require this module explicitly, use require('buffer/') which tells the node.js module lookup algorithm (also used by browserify) to use the npm module named buffer instead of the node.js core module named buffer!

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no buffer package being imported. Am I missing some point?

@daviddias daviddias changed the title adds failing test: ifps.files.add NPM buffer adds failing test: ipfs.files.add NPM buffer Dec 30, 2017
@daviddias
Copy link
Contributor

@hacdias mind checking this one?

@hacdias
Copy link
Contributor

hacdias commented Jan 7, 2018

@joaosantos15 @diasdavid I've got one thing to ask: we support the native Node.js Buffer. So, why are trying to support feross/buffer? This way, we should support every single Buffer package which is on NPM.

Why don't we do something similar to what js-ipfs does? Just make Buffer part of the type so when someone uses it in the browser they can use ipfsApi.types.Buffer?

@hacdias
Copy link
Contributor

hacdias commented Jan 9, 2018

@diasdavid could you take a look at my previous comment? 😄

@daviddias
Copy link
Contributor

@hacdias the feross/Buffer package is the browser shim that every Bundler (browserify, webpack, etc) uses to shim the Node.js Buffer.

I started getting this issue as well on #688

image

I believe @vmx had found problems with feross/Buffer package. @vmx will feross/buffer#178 fix this issue?

@vmx
Copy link
Contributor

vmx commented Feb 10, 2018

@diasdavid I don't think feross/buffer#178 will fix the issue. Though I think that once feross/buffer#177 is done, it might fix the issue (178 is just a small part of that. But I won't spend more time on it until I got feedback on it).

@daviddias daviddias changed the title adds failing test: ipfs.files.add NPM buffer Buffer shim is no longer working in the Browser as expected Feb 12, 2018
@daviddias daviddias changed the base branch from master to buffer-issue February 12, 2018 12:31
@ghost ghost assigned daviddias Feb 12, 2018
@ghost ghost added in progress and removed ready labels Feb 12, 2018
@daviddias
Copy link
Contributor

merging into a branch so that we all can edit.

@daviddias daviddias merged commit 80827bf into ipfs-inactive:buffer-issue Feb 12, 2018
@ghost ghost removed the in progress label Feb 12, 2018
@daviddias
Copy link
Contributor

Follow on #689

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

Successfully merging this pull request may close these issues.

4 participants