modularize js-ipfs-api #561

nunofmn · 2017-06-05T16:36:02Z

Related to #544.

The structure will be similar to the async module structure, but to do this with minimal changes to the current structure, is necessary a "module init" step where the config is passed to the request-api inner module.

The usage will be similar to the following:

const bitswap = require('ipfs-api/src/bitswap')('/ip4/127.0.0.1/tcp/5001')

bitswap.unwant(key, (err) => {
  // ...
}

dignifiedquire · 2017-06-07T15:43:48Z

src/api/bitswap.js

-const promisify = require('promisify-es6')
-
-module.exports = (send) => {
+module.exports = (arg) => {


might be nicer to rename to bitswap/index.js

Initially I had some problems when I structured it like you are saying. But I don´t remember what was the problem. I will try again 😃

dignifiedquire · 2017-06-07T15:45:16Z

src/api/bitswap/stat.js

+    send = requestAPI(arg)
+  } else {
+    throw new Error('Argument must be a send function or a config object.')
+  }


seems like the above boilerplate should be extracted into its own file and be reused, rather than copied into each file

Yes, I was thinking in extracting the config processing part in the IpfsAPI function, and use it in all modules.

dignifiedquire · 2017-06-07T15:49:07Z

test/bitswap.spec.js

@@ -50,13 +68,39 @@ describe('.bitswap', () => {
      })
    })

+    it('.stat (modular)', (done) => {


It would be nice to find a way to not duplicate the tests, but rather reuse them and just pass in the two different versions

Do you have any idea of how this could be done?

daviddias · 2017-06-08T15:47:07Z

@nunofmn I like this approach, it builds nicely. Follow @dignifiedquire suggesting, making it modular should be about importing only what you need but also avoiding code duplication.

Thanks! 🌟

daviddias · 2017-06-16T18:19:59Z

Hi @nunofmn, any hopes of getting this PR finished? This will make the life so much easier for everyone and I would love to get this merged before other PR's are made :)

daviddias

Lot's of good progress here @nunofmn !! :D I'm super excited about this PR (as you know :D)

daviddias · 2017-06-21T15:17:14Z

README.md

@@ -224,7 +224,12 @@ This means:
 - [`ipfs.id([callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic#id)
 - [`ipfs.version([callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic#version)
 - [`ipfs.ping()`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic#ping)
- [`ipfs.log()`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic#log)
+
+#### [log](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic)


Let's remove these headings.

See how it is done in line 212

daviddias · 2017-06-21T15:17:21Z

README.md

+
+- [`ipfs.log.ls([callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic)
+- [`ipfs.log.tail([callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic)
+- [`ipfs.log.level(subsystem, level, [options, callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/API/generic)

 #### [key](https://github.com/ipfs/interface-ipfs-core/tree/master/API/key)


Let's remove these headings.

See how it is done in line 212

daviddias · 2017-06-21T15:17:54Z

src/api/bitswap/index.js

+    stat: require('./stat')(arg),
+    unwant: require('./unwant')(arg)
+  }
+}


This needs to be bubbled up.

src/bitswap/index.js and not src/api/bitswap/index.js

daviddias · 2017-06-21T15:20:47Z

src/api/bitswap/stat.js

+const moduleConfig = require('../../module-config')
+
+module.exports = (arg) => {
+  const send = moduleConfig(arg)


Do this step at the index.js level. Otherwise, you are creating a 'send' instance for every API call.

And also, at the index.js, you should check if the arg is already a send instance, for when the user uses ipfs-api directly and not a subapi.

The main problem of doing this step at index.js level is that will lose the capability of require individual sub-modules (for example, require('ipfs-api/bitswap/unwant')(config) )

The moduleConfig already checks if the arg is a send instance, or if is necessary to init one.

daviddias · 2017-06-21T15:21:24Z

src/api/block/get.js

+      }
+    } catch (err) {
+      return callback(err)
+    }


Could you refactor this into a function?

daviddias · 2017-06-21T15:26:00Z

As you move all the /src/api/* to /src/*, move all the other files in (https://github.com/ipfs/js-ipfs-api/tree/master/src) to /src/utils/*
Please create an example showing how to require just a subAPI

daviddias · 2017-06-22T14:31:58Z

src/module-config.js

+  } else {
+    throw new Error('Argument must be a send function or a config object.')
+  }
+}


This doesn't seem to be correct. What if a user does const node = ipfsApi('/ip4/127.0.0.1/tcp/5001') ?

daviddias · 2017-06-26T08:01:26Z

@nunofmn almost there :)

resolve merge conflict
git ignore the generated files in the example
update the readme

daviddias · 2017-06-26T08:25:05Z

examples/sub-module/package.json

+    "babel-preset-babili": "^0.1.4",
+    "babel-preset-env": "^1.5.2",
+    "babili": "^0.1.4",
+    "babili-webpack-plugin": "^0.1.1",


Why so much babel? Shouldn't this module require any babel at all?

daviddias · 2017-06-26T08:27:11Z

src/bitswap/stat.js

@@ -5,7 +5,7 @@ const promisify = require('promisify-es6')
 module.exports = (send) => {
  return promisify((callback) => {
    send({
-      path: 'commands'
+      path: 'bitswap/stat'
    }, callback)
  })
 }


Interesting that git thinks this was a rename from commands.js

daviddias · 2017-06-26T11:13:52Z

Values in KBs

Good results! Once we get ipfs/kubo#4008 it will even be better! :)

daviddias · 2017-06-26T12:50:36Z

examples/sub-module/bundles-size-KBs.csv

+repo - 684.56 - 232.02
+swarm - 1324.18 - 527.03
+update - 684.45 - 231.96
+version - 684.21 - 231.88


Please use commas in the comma separated values files :)

Also, add the units to the top.

victorb · 2017-06-26T16:09:20Z

examples/sub-module/test-modules-size.sh

@@ -0,0 +1,25 @@
+#!/bin/sh
+


Should have at least set -e so it exits the entire script if there is a error

daviddias · 2017-06-27T16:07:27Z

This has been an impressive refactor! Thank you @nunofmn for pulling this off.

There is one failing test, not related with the changes itself. Needs to be investigated separately. Merging!

jbenet added the in progress label Jun 5, 2017

dignifiedquire reviewed Jun 7, 2017

View reviewed changes

daviddias assigned nunofmn Jun 16, 2017

daviddias suggested changes Jun 21, 2017

View reviewed changes

daviddias reviewed Jun 22, 2017

View reviewed changes

daviddias mentioned this pull request Jun 25, 2017

reduce size of ipfs.add bundle and number of RPC requests. #522

Closed

daviddias suggested changes Jun 26, 2017

View reviewed changes

refactor: modularize js-ipfs-api(ipfs-inactive#544)

4d810a0

nunofmn force-pushed the 544-make-modular branch from c67c760 to 4d810a0 Compare June 26, 2017 10:59

daviddias approved these changes Jun 26, 2017

View reviewed changes

nunofmn added 2 commits June 26, 2017 12:13

examples: more descriptive name in sub-module example csv.

f061df9

refactor: add add/cat/get sub-modules.(ipfs-inactive#544)

9581ce0

daviddias suggested changes Jun 26, 2017

View reviewed changes

nunofmn force-pushed the 544-make-modular branch from d1dedc6 to 75f532d Compare June 26, 2017 13:32

examples: fix csv generation in submodules example.

34d95a8

nunofmn force-pushed the 544-make-modular branch from 75f532d to 34d95a8 Compare June 26, 2017 13:39

daviddias requested review from victorb and dignifiedquire June 26, 2017 15:43

daviddias approved these changes Jun 26, 2017

View reviewed changes

victorb reviewed Jun 26, 2017

View reviewed changes

fix: shell script in sub-modules example.

c25d13a

victorb approved these changes Jun 26, 2017

View reviewed changes

fix: lint errors in examples.

e1b5b12

readme: update sub-modules usage.

7122c18

daviddias changed the title ~~[WIP] Modularize js-ipfs-api~~ modularize js-ipfs-api Jun 27, 2017

daviddias merged commit 2970c9f into ipfs-inactive:master Jun 27, 2017

daviddias removed the in progress label Jun 27, 2017

daviddias mentioned this pull request Jun 27, 2017

Make js-ipfs-api modular #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modularize js-ipfs-api #561

modularize js-ipfs-api #561

nunofmn commented Jun 5, 2017 •

edited

Loading

dignifiedquire Jun 7, 2017

daviddias Jun 8, 2017

nunofmn Jun 17, 2017

dignifiedquire Jun 7, 2017

daviddias Jun 8, 2017

nunofmn Jun 17, 2017

dignifiedquire Jun 7, 2017

daviddias Jun 8, 2017

nunofmn Jun 17, 2017

daviddias commented Jun 8, 2017

daviddias commented Jun 16, 2017

daviddias left a comment

daviddias Jun 21, 2017

daviddias Jun 21, 2017

daviddias Jun 21, 2017

daviddias Jun 21, 2017

nunofmn Jun 21, 2017 •

edited

Loading

daviddias Jun 21, 2017

daviddias commented Jun 21, 2017 •

edited by nunofmn

Loading

daviddias Jun 22, 2017

daviddias commented Jun 26, 2017 •

edited by nunofmn

Loading

daviddias Jun 26, 2017

daviddias Jun 26, 2017

daviddias commented Jun 26, 2017

daviddias Jun 26, 2017

victorb Jun 26, 2017 •

edited

Loading

daviddias commented Jun 27, 2017

modularize js-ipfs-api #561

modularize js-ipfs-api #561

Conversation

nunofmn commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jun 8, 2017

daviddias commented Jun 16, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunofmn Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jun 21, 2017 • edited by nunofmn Loading

Choose a reason for hiding this comment

daviddias commented Jun 26, 2017 • edited by nunofmn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jun 26, 2017

Choose a reason for hiding this comment

victorb Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

daviddias commented Jun 27, 2017

nunofmn commented Jun 5, 2017 •

edited

Loading

nunofmn Jun 21, 2017 •

edited

Loading

daviddias commented Jun 21, 2017 •

edited by nunofmn

Loading

daviddias commented Jun 26, 2017 •

edited by nunofmn

Loading

victorb Jun 26, 2017 •

edited

Loading