Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: move ipfs to peer deps #446

Merged
merged 6 commits into from
Jan 28, 2020
Merged

feat: move ipfs to peer deps #446

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1cd001
refactor: move ipfs to peer deps
achingbrain Jan 24, 2020
0606d3b
fix: install peer deps before bundle size check
achingbrain Jan 24, 2020
f9eb5ed
fix: do not error if go-ipfs-dep is not installed
achingbrain Jan 24, 2020
95a7bad
fix: conditionally require ipfs based on factory type
achingbrain Jan 25, 2020
44f1431
fix: only import ipfs if it has not been specifed
achingbrain Jan 27, 2020
6adceb9
fix: requir ipfs when spawning
achingbrain Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,20 @@ os:
- osx
- windows

script: npx nyc -s npm run test:node -- --timeout 60000
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For discussion: we should install the versions this module depends on otherwise updating them may cause the tests for this module to break.

Copy link
Member Author

@achingbrain achingbrain Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do, though this module depends on * so it already does install the version this module depends on, for better or for worse :trollface:

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.

- npx nyc -s npm run test:node -- --timeout 60000
after_success: npx nyc report --reporter=text-lcov > coverage.lcov && npx codecov

jobs:
include:
- stage: check
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
- npx aegir build --bundlesize
- npx aegir commitlint --travis
- npx aegir dep-check
Expand All @@ -31,25 +38,37 @@ jobs:
addons:
chrome: stable
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
- npx aegir test -t browser -t webworker --bail --timeout 60000

- stage: test
name: firefox
addons:
firefox: latest
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
- npx aegir test -t browser -t webworker --bail --timeout 60000 -- --browsers FirefoxHeadless

- stage: test
name: electron-main
os: osx
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
- npx aegir test -t electron-main --bail --timeout 60000

- stage: test
name: electron-renderer
os: osx
script:
- npm install ipfs
- npm install ipfs-http-client
- npm install go-ipfs-dep
- npx aegir test -t electron-renderer --bail --timeout 60000
notifications:
email: false
12 changes: 12 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ Version 1.0.0 changed a bit the api and the options methods take so please read
npm install --save ipfsd-ctl
```

Please ensure your project also has dependencies on `ipfs`, `ipfs-http-client` and `go-ipfs-dep`.

```sh
npm install --save ipfs
npm install --save ipfs-http-client
npm install --save go-ipfs-dep
```

If you are only going to use the `go` implementation of IPFS, you can skip installing the `js` implementation and vice versa, though both will require the `ipfs-http-client` module.

If you are only using the `proc` type in-process IPFS node, you can skip installing `go-ipfs-dep` and `ipfs-http-client`.

## Usage

### Spawning a single IPFS controller: `createController`
Expand Down
8 changes: 5 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@
"debug": "^4.1.1",
"execa": "^4.0.0",
"fs-extra": "^8.1.0",
"go-ipfs-dep": "~0.4.22",
"ipfs": "0.40.0",
"ipfs-http-client": "^41.0.0",
"ipfs-utils": "^0.7.1",
"ky": "^0.16.1",
"ky-universal": "^0.3.0",
Expand All @@ -77,6 +74,11 @@
"husky": "^4.0.10",
"lint-staged": "^10.0.2"
},
"peerDependencies": {
"go-ipfs-dep": "*",
"ipfs": "*",
"ipfs-http-client": "*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

},
"repository": {
"type": "git",
"url": "git+https://github.com/ipfs/js-ipfsd-ctl.git"
Expand Down
19 changes: 11 additions & 8 deletions src/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ const defaults = {
path: require.resolve('ipfs-http-client'),
ref: require('ipfs-http-client')
},
ipfsModule: {
path: require.resolve('ipfs'),
ref: require('ipfs')
},
ipfsBin: findBin('go'),
ipfsOptions: {}
}

Expand All @@ -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 when none has been specifed
if ((this.opts.type === 'js' || this.opts.type === 'proc') && !this.opts.ipfsModule) {
this.opts.ipfsModule = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will overwrite ipfsModule that the user has passed in. We should only set this if this.opts.ipfsModule is not yet set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout, have pushed a fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just to avoid throwing when spawning go controllers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when your project is only going to use go-IPFS there's not much point installing js-IPFS as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved that clause to .spawn so it's run against the merged options

path: require.resolve('ipfs'),
ref: require('ipfs')
}
}

/** @type ControllerOptionsOverrides */
this.overrides = merge({
js: merge(this.opts, { type: 'js', ipfsBin: findBin('js') }),
go: this.opts,
proc: merge(this.opts, { type: 'proc', ipfsBin: findBin('js') })
js: merge(this.opts, { type: 'js', ipfsBin: findBin('js', this.opts.type === 'js') }),
go: merge(this.opts, { type: 'go', ipfsBin: findBin('go', this.opts.type === 'go') }),
proc: merge(this.opts, { type: 'proc', ipfsBin: findBin('js', this.opts.type === 'proc') })
}, overrides)

/** @type ControllerDaemon[] */
Expand Down
8 changes: 5 additions & 3 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ const checkForRunningApi = (repoPath) => {
return api ? api.toString() : null
}

const findBin = (type) => {
const findBin = (type, required) => {
const resolve = required ? resolveCwd : resolveCwd.silent

if (type === 'js') {
return process.env.IPFS_JS_EXEC || resolveCwd('ipfs/src/cli/bin.js')
return process.env.IPFS_JS_EXEC || resolve('ipfs/src/cli/bin.js')
}

return process.env.IPFS_GO_EXEC || resolveCwd(`go-ipfs-dep/go-ipfs/${isWindows ? 'ipfs.exe' : 'ipfs'}`)
return process.env.IPFS_GO_EXEC || resolve(`go-ipfs-dep/go-ipfs/${isWindows ? 'ipfs.exe' : 'ipfs'}`)
}

const tmpDir = (type = '') => {
Expand Down