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

Browserify integration is broken #190

Closed
area opened this issue Jan 19, 2016 · 17 comments
Closed

Browserify integration is broken #190

area opened this issue Jan 19, 2016 · 17 comments

Comments

@area
Copy link

area commented Jan 19, 2016

I have been using the example provided at https://github.com/ipfs/js-ipfs-api for a proof-of-concept, successfully communicating with an IPFS node hosted locally for the last week or so. Earlier today, I blew away my node_modules directory and reinstalled the relevant packages (i.e. js-ipfs-api). The example now no longer works for me - I get an error in my console:

Uncaught Error: options.payload must be a string, a Buffer, or a Stream
exports.contain.exports.reachTemplate.exports.assert.condition @ app.js:36055
internals.Client.request.onError @ app.js:47850
requestAPI @ app.js:37627add @ app.js:36985
ColonyJS.store @ app.js:29396
ColonyJS.makeProposal @ app.js:29314
(anonymous function) @ app.js:29265

I am aware that due to my use of browserify, these line numbers are not useful! Some investigation reveals, I believe, that something fishy is happening around https://github.com/ipfs/js-ipfs-api/blob/285a7e3746110c1fe843b938c026161856c36bcd/src/request-api.js#L83-L85 . files is a type Buffer using the code, but the stream that is returned that I see is of type Duplexify. I have not burrowed deep into the dependencies to figure out why this is happening, but it is this conversion that means that the check conducted by Wreck (https://github.com/hapijs/wreck/blob/238647bc9b45120457e35adb85061586b796e0a7/lib/index.js#L71-L73) fails, throwing the error.

I will note with interest that using the wrapper for js-ipfs-api at https://github.com/ConsenSys/ipfs.js the example continues to work. I assume this is due to their precise versioning in their package.json, and something has inadvertently implemented a breaking change, somewhere.

@daviddias
Copy link
Contributor

What is the IPFS version you are currently running? ipfs.js is locked to [email protected], my guess is that you are running a older version of IPFS that has a different HTTP API than the latest release.

@ckeenan
Copy link
Contributor

ckeenan commented Jan 21, 2016

I see this error with the browserified code as well. Tried with builds for 0.3.10, 0.3.11, 0.4.0-dev

browserify -s ipfsAPI -e ./src/index.js > ipfs.js

<!doctype html>
<html lang="en">
    <body>
        <script src="./ipfs.js"></script>
        <script>
            var ipfs = ipfsAPI('localhost', 5001);
            ipfs.add(new ipfs.Buffer('testing'), function(err, res) {
                console.log('err, res', err, res);
            });
        </script>
    </body>
</html>

@area
Copy link
Author

area commented Jan 21, 2016

I'm using IPFS 0.3.10 (installed from homebrew on OSX, if that is ultimately relevant)

@daviddias
Copy link
Contributor

@daviddias daviddias added the bug label Jan 21, 2016
@ckeenan
Copy link
Contributor

ckeenan commented Jan 21, 2016

@diasdavid I can include a dist file in <script > and that works fine. But trying to require a browserify-ed version of src does not

@daviddias
Copy link
Contributor

@ckeenan our dist'ed version suffers some transformations, straight up browserify doesn't work anymore, to check what webpack uses/does, see https://github.com/ipfs/js-ipfs-api/blob/master/tasks/config.js

@egasimus
Copy link

egasimus commented Feb 1, 2016

Having variations of the same issue when using Browserify. Tried passing in a File, an ArrayBuffer, a maxogden/filereader-stream, to no avail. Unlikely to have anything to do with the server imho, looks totally like a client side thing caused by Vinyl's type assertions.

@egasimus
Copy link

egasimus commented Feb 1, 2016

Having just gone through a git bisect, it turns out that the commit where the browserified library stops recognizing Browserify buffers is, perhaps somewhat unsurprisingly, the one where Browserify is replaced with Webpack. Any thoughts on what could've gone wrong @diasdavid, @dignifiedquire?

@dignifiedquire
Copy link
Contributor

@egasimus are you using the dist version or are you using a browserified version of src?

@egasimus
Copy link

egasimus commented Feb 1, 2016

@dignifiedquire I'm browserifying it myself.

@daviddias
Copy link
Contributor

@dignifiedquire since we had written on the README that browserify is the 'go to' solution to use js-ipfs-api in the browser, we should either 1) remove it (if it is broken completely) and replace by documentation on how to use it with WebPack or 2) even better, make it work with browserify and Webpack and give examples for it (plus tests)

@jbenet
Copy link
Contributor

jbenet commented Feb 3, 2016

i'd much prefer (2) o/

@dignifiedquire
Copy link
Contributor

I talked to @diasdavid and the next action points are:

  1. Run all tests through browserify as well as through webpack
  2. Setup examples for using js-ipfs-api with webpack and browserify and have them run on CI so we are sure they are up to date and working.
  3. Update the README with instructions for both webpack and browserify and pointing to the examples.

@dignifiedquire dignifiedquire changed the title Example no longer works Browserify integration is broken Feb 3, 2016
@dignifiedquire
Copy link
Contributor

I figured out the issue why this is failing: vinyl checks for isStream with instanceof Stream but this does not work in browserify with a PassThrough stream (https://github.com/gulpjs/vinyl/blob/master/lib/isStream.js#L4) I'm not sure how to solve/fix this and we are going to drop vinyl in the 0.4. release anyway which should make this issue go away.

@egasimus
Copy link

egasimus commented Feb 6, 2016

Good to see progress on this. 👍 How do you plan to replace Vinyl?

@daviddias
Copy link
Contributor

We've just released a new version (3.0.1) and also updated the installation docs, you should not encounter any more problem using this module with browserify. Let us know if it is working for you.

@daviddias
Copy link
Contributor

Closing this one, as it should not be a problem anymore. Let us know if you still have an issue :)

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

No branches or pull requests

6 participants