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

Refactor: IPFS dependencies and running tests stand-alone #8

Closed
wants to merge 7 commits into from
Closed

Refactor: IPFS dependencies and running tests stand-alone #8

wants to merge 7 commits into from

Conversation

haadcode
Copy link

This PR will change the dependencies so that instead of using ipfs-js and browser-ipfs, the module uses and instance of IPFS acquired via ipfs-api. That is, one passes an instance of IPFS in registry.setIpfsProvider(ipfs).

This PR will also refactor tests so that they can be run stand-alone. Included now as a dev dependencies are ethereumjs-testrpc and ipfsd-ctl which both provide a way to start the required daemons programmatically. I added a test/testutils.js file that starts the daemons.

I also refactored the setAttributes/getAttributes functions in uportregistry.js to use the canonical IPFS API (ie. ipfs-api).

@haadcode haadcode mentioned this pull request Aug 18, 2016
@pelle
Copy link
Contributor

pelle commented Aug 18, 2016

We need to make sure that this works in the browser as well using an instance of browser-ipfs

@haadcode
Copy link
Author

I haven't verified yet it works in the browser but I don't see why not. I'll verify and make sure it works. Is there a specific test case somewhere I can use to test? An example perhaps?

Can you elaborate on the need for browser-ipfs? Using js-ipfs-api should work in the browser out of the box.

@haadcode
Copy link
Author

It all works (uport-*) in the browser. Same for ipfs.

@pelle
Copy link
Contributor

pelle commented Aug 18, 2016

Primary reason for using browser-ipfs:

Lots of unnecessary dependencies cause fragility and huge integration problems both in browser and ReactNative.

Secondary reason is size, which isn't an issue in ReactNative but a very big issue on the web in particular mobile web:

js-ipfs-api

99% 2016-08-18 11:38:01 ☆  |2.3.0| Big-Corn-Island in ~/code/js-ipfs-api
± |master ✓| → ls -l dist/
total 8600
-rw-r--r--  1 pelleb  staff  1550487 Aug 18 11:36 index.js
-rw-r--r--  1 pelleb  staff  1925637 Aug 18 11:36 index.js.map
-rw-r--r--  1 pelleb  staff   920487 Aug 18 11:36 index.min.js

browser-ipfs

± |master → origin {2} ✓| → ls -l dist/
total 8
-rw-r--r--  1 pelleb  staff  1592 Apr 25 21:53 ipfs.min.js

@haadcode
Copy link
Author

Very valid points!

We're working on reducing the size of js-ipfs and I've forwarded your feedback to the team here ipfs/js-ipfs#398, hopefully the same feedback is taken into consideration and we can optimize js-ipfs-api, too.

Looking at browser-ipfs, it is indeed nice and small! Which raises the question, why is js-ipfs-api ~900kb and not ~2kb :)

Re. ipfs-js and browser-js, is there a specific reason you're converting the data to files before adding it to IPFS? As far as I can tell, you'd be fine with using ipfs.object.put/get to store the data as objects (as opposed to files), the size limit is 256kb per object.

Since you want to use a very light ipfs-api lib, this PR doesn't make that much sense on the IPFS dependencies part. I'll see what I can do to change this. The fixes to the tests are still valid, no?

@oed
Copy link
Contributor

oed commented Aug 19, 2016

Yeah, I like the changes made to the tests.

@pelle
Copy link
Contributor

pelle commented Jan 4, 2017

@haadcode I'm going to close this for now. I think we've pretty much implemented what you need, just with support for browsers through ipfs-mini instead.

@pelle pelle closed this Jan 4, 2017
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.

3 participants