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

Conversation

@krl
Copy link
Contributor

@krl krl commented Aug 29, 2015

In order to make add work in the browser with the standalone browser, we need to access the Buffer object, it seems importing the browser-buffer as standalone trips up vinyl somehow.

This is not ideal, but unless we can find a way to make adding files from buffers really portable, it seems ok.

The three cases are:

  • node, just works
  • browserify, works
  • standalone module in browser, not without exposing browserifys Buffer

@jbenet
Copy link
Contributor

jbenet commented Aug 29, 2015

@diasdavid will know better than me how to handle this

@daviddias
Copy link
Contributor

It is a pity that both Buffers are called Buffers and don't have the implementation.

Since this is not a common practice and also something not so usual to do in Browser applications, I suggest strongly that it has to be documented in the README and in case a user calls the function with a normal Browser Buffer obj, we should throw an error an point to the link in the README that explains that scenario.

One other suggestion, which I believe it would be more clean, but at the same time, less convenient, is instead of exposing the buffer-browserify through self, invite people to use https://github.com/feross/buffer, which makes a very good job explaining the differences, making this step a 'buy in' from developers, ensuring that everyone knows the differences and also that they understand this 'special' Buffer can be used for scenarios than just the node-ipfs-api.

@krl
Copy link
Contributor Author

krl commented Aug 30, 2015

We could also maybe change the default behavior of interpreting strings as filenames, and explicitly require Vinyl files, this way we could simply add JSON.stringify(object), without any extra headache.

I'm not getting ipfs object add to work with Buffer included from either npm browser-buffer or buffer, at least not in combination with webcomponents.

@krl
Copy link
Contributor Author

krl commented Aug 30, 2015

For now, i would think that exposing Buffer is not the worst way to go. It works reliably, even in webcomponents.

@diasdavid

@daviddias
Copy link
Contributor

Ok :) just add some documentation to it as other people will try to understand why their browser-buffers aren't working.

@jbenet
Copy link
Contributor

jbenet commented Aug 30, 2015

We could also maybe change the default behavior of interpreting strings as filenames, and explicitly require Vinyl files, this way we could simply add JSON.stringify(object), without any extra headache.

no, that changes the interface. we promise that:

ipfs add <path>
ipfs.add(<path>) // js
ipfs.add(<path>) // go

all work the same: adds the file addressed by <path>

krl added a commit that referenced this pull request Aug 31, 2015
@krl krl merged commit a518831 into master Aug 31, 2015
@daviddias
Copy link
Contributor

Oh :(, no documentation added to the README?

@jbenet jbenet deleted the expose-buffer branch August 31, 2015 21:32
@harlantwood
Copy link
Contributor

Probably worth pushing out an NPM release for this, it's quite useful.

@daviddias
Copy link
Contributor

@harlantwood you are totally right. Pushed 2.3.1 tag to npm https://github.com/ipfs/node-ipfs-api/releases/tag/v2.3.1

@krl
Copy link
Contributor Author

krl commented Sep 3, 2015

@diasdavid pushed 2.3.2, i think you forgot to run npm run build before!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants