feat: move ipfs to peer deps #446
Conversation
Doing this reduced the size of `node_modules` in the js-IPFS module for me by about 300MB. Does require the user to bring their own version, but they can also skip installing go-IPFS if, for example, they are only going to use js-IPFS.
achingbrain
force-pushed
the
move-ipfs-to-peer-deps
branch
from
5f91a4f
to
e1cd001
Compare
| "peerDependencies": { | ||
| "go-ipfs-dep": "*", | ||
| "ipfs": "*", | ||
| "ipfs-http-client": "*" |
There was a problem hiding this comment.
...but I think you'll have to install all of these even if you use only 1 because the calls to require will fail? Do we need try/catches?
There was a problem hiding this comment.
Quite right. I think you'll need ipfs-http-client all the time, even the proc factory type needs it because it checks to see if another ipfs process has opened the repo, if it has then it uses the http client to connect to it.
I've pushed a change that only requires ipfs if we're going to use it. go-ipfs-dep isn't required directly, instead the findBin function in utils.js tries to pull the binary path out of the module so I've made that fail silently if the factory type is not 'go'.
src/factory.js
Outdated
| @@ -46,11 +41,19 @@ class Factory { | |||
| /** @type ControllerOptions */ | |||
| this.opts = merge(defaults, options) | |||
|
|
|||
| // conditionally include ipfs based on which type of daemon we will spawn | |||
| if (this.opts.type === 'js' || this.opts.type === 'proc') { | |||
| this.opts.ipfsModule = { | |||
There was a problem hiding this comment.
This will overwrite ipfsModule that the user has passed in. We should only set this if this.opts.ipfsModule is not yet set.
There was a problem hiding this comment.
Good shout, have pushed a fix
There was a problem hiding this comment.
this is just to avoid throwing when spawning go controllers?
There was a problem hiding this comment.
Yes, when your project is only going to use go-IPFS there's not much point installing js-IPFS as well.
There was a problem hiding this comment.
this still fails when bundling (or at least warns depending on the bundler) and if the user wants to use js or proc they are forced to specify it in the factory constructor or else they get an error when calling .spawn({type:'proc'})
There was a problem hiding this comment.
I've moved that clause to .spawn so it's run against the merged options
| script: | ||
| - npm install ipfs | ||
| - npm install ipfs-http-client | ||
| - npm install go-ipfs-dep |
There was a problem hiding this comment.
For discussion: we should install the versions this module depends on otherwise updating them may cause the tests for this module to break.
There was a problem hiding this comment.
Can do, though this module depends on * so it already does install the version this module depends on, for better or for worse 
We could change it to be an actual version, but that would imply we require a certain version.
On that, looking through the code quickly, the API methods we call are:
ipfs.id
ipfs.version
ipfs.init
ipfs.start
ipfs.stop
I guess it's feasible that we could refactor those, but they are pretty low-churn.
The .id invocation looks a bit weird to me - it's used to set a peerId property on a node which is then used by the tests so we could probably remove it (in another PR).
We could probably remove .version too, as the caller can just invoke the API method themselves.
There was a problem hiding this comment.
LGTM, still needs a BREAKING CHANGE message but I guess @hugomrdias can add on squashmerge.
src/factory.js
Outdated
| @@ -46,11 +41,19 @@ class Factory { | |||
| /** @type ControllerOptions */ | |||
| this.opts = merge(defaults, options) | |||
|
|
|||
| // conditionally include ipfs based on which type of daemon we will spawn | |||
| if (this.opts.type === 'js' || this.opts.type === 'proc') { | |||
| this.opts.ipfsModule = { | |||
There was a problem hiding this comment.
this is just to avoid throwing when spawning go controllers?
src/factory.js
Outdated
| @@ -46,11 +41,19 @@ class Factory { | |||
| /** @type ControllerOptions */ | |||
| this.opts = merge(defaults, options) | |||
|
|
|||
| // conditionally include ipfs based on which type of daemon we will spawn | |||
| if (this.opts.type === 'js' || this.opts.type === 'proc') { | |||
| this.opts.ipfsModule = { | |||
There was a problem hiding this comment.
this still fails when bundling (or at least warns depending on the bundler) and if the user wants to use js or proc they are forced to specify it in the factory constructor or else they get an error when calling .spawn({type:'proc'})
|
@hugomrdias I think I've addressed everything - is this good to go? |
Doing this reduced the size of
node_modulesin the js-IPFS module for me by about 300MB and should result in a faster CI run for js-IPFS since it doesn't need to install itself.Does require the user to bring their own version, but they can also skip installing go-IPFS if, for example, they are only going to use js-IPFS.