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

Remove or change the ipfs-http-client dependency? #374

Closed
achingbrain opened this issue Sep 13, 2019 · 2 comments
Closed

Remove or change the ipfs-http-client dependency? #374

achingbrain opened this issue Sep 13, 2019 · 2 comments
Labels
exp/wizard Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P0 Critical: Tackled by core team ASAP

Comments

@achingbrain
Copy link
Member

We bundle ipfs-http-client with this module, yet require implementations of go-ipfs and js-ipfs to either be passed to the factory (for in-proc nodes) or installed on the system for (daemon nodes).

This can lead to the situation where you have an ipfs-http-client that is incompatible with the ipfs version being used due to deps being out of date or miscellaneous API changes and consequently introduces a lot of release churn just for dep updates.

We can pass an implementation of ipfs-http-client in the same way as the js-ipfs implementation, but this relies on the developer doing the right thing - reading the docs, passing the arg, etc and gives you extra dependencies you aren't going to use.

We are not the only consumers of this library, people seem to use it as a convenient way to start up a node: https://github.com/ipfs/js-ipfsd-ctl/network/dependents?package_id=UGFja2FnZS0xNDQ1MDU1Mw%3D%3D Though we did just go from callbacks -> promises without much complaint so people are either not upgrading or don't mind the odd breaking change.

We can:

  1. Do nothing
  2. Change ipfs-http-client to be a peerDependency
  3. Remove it entirely and require an implementation, similar to how we require an implementation for an in-proc node

Extracted from ipfs/js-ipfs#2446
@achingbrain
Copy link
Member Author

In the case of 1. Do nothing, it adds a gotcha to the use of this module so we then have to continually explain to everyone that our module does the wrong thing by default and you should really not rely on the bundled ipfs-http-client and instead you should specify your own and etc etc etc...

@achingbrain
Copy link
Member Author

Here's someone discovering the mismatch on their own and encountering surprise: orbitdb-archive/ipfs-log#266 (comment)

@hugomrdias hugomrdias added exp/wizard Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P0 Critical: Tackled by core team ASAP labels Sep 26, 2019
@achingbrain achingbrain mentioned this issue Oct 17, 2019
Closed
@hugomrdias hugomrdias mentioned this issue Nov 8, 2019
Merged
7 tasks
hugomrdias added a commit that referenced this issue Dec 11, 2019
@hugomrdias
feat: new ctl (#395)
3dae276 
This is a complete new version of `ipfsd-ctl` dragons ahead!

BREAKING CHANGE:


Problems: 
- Browsers tests skipped cause ctl didn't support proper connectivity to remote nodes
- We weren't able to tell ctl to use a specific commit of http-client, js-ipfs or cli
- Options/config between the 3 types of daemons weren't consistent
- Ctl didn't support remote "in process" daemon
- IPFS options were handled manually inside ctl, so any change in js-ipfs would require a PR in ctl to support the new options or change to an option

Related issues:
- #208
- #397
- #374
- #315
- #207
- #217
- and more 
 
Improvements:
- better errors
- DEBUG='ipfsd-ctl:*' everywhere
- factories for tests with good defaults
- options are properly merged everywhere
- safer child_process exit `stop()`
- faster stop()
- IPFS Options are now the same format as https://github.com/ipfs/js-ipfs/blob/master/README.md#ipfs-constructor
- Ctl can init, start and set config in one cmd (at least with js-ipfs)
- better docs and jsdocs
- we can now be sure which http-client, ipfs or go-ipfs is being used
- utils functions actually work in the browser now
- works in webworkers now
- simpler and faster overall
- disposable node actually clean themselves in the browser
- better tests
- ...
- support electron
- test in electron

New: 
- new method `createController` returns a spawned controller
- createFactory as a second parameter to override options per type


Changes: 
- `create` change to `createFactory`
- `createFactory` options changed   

Old
```md
- `options` - optional object with:
  - `remote` bool - use remote endpoint to spawn the nodes.
  - `port` number - remote endpoint port. Defaults to 43134.
  - `exec` - IPFS executable path. `ipfsd-ctl` will attempt to locate it by default. If you desire to spawn js-ipfs instances in the same process, pass the ref to the module instead (e.g `exec: require('ipfs')`)
  - `type` - the daemon type, see below the options
    - `go` - spawn go-ipfs daemon
    - `js` - spawn js-ipfs daemon
    - `proc` - spawn in-process js-ipfs instance. Needs to be called also with exec. Example: `DaemonFactory.create({type: 'proc', exec: require('ipfs') })`.
  - `IpfsClient` - A custom IPFS API constructor to use instead of the packaged one
```

**New**
```markdown
-   `remote` [boolean] Use remote endpoint to spawn the nodes. Defaults to `true` when not in node.
-   `test` [test=false] - Flag to activate custom config for tests.
-   `endpoint` [endpoint] - Endpoint URL to manage remote Controllers. (Defaults: 'http://localhost:43134').
-   `disposable` [Boolean] A new repo is created and initialized for each invocation, as well as cleaned up automatically once the process exits.
-   `type` [string] The daemon type, see below the options:-   go - spawn go-ipfs daemon
    -   js - spawn js-ipfs daemon
    -   proc - spawn in-process js-ipfs instance
-   `env` [Object] Additional environment variables, passed to executing shell. Only applies for Daemon controllers.
-   `args` [Array] Custom cli args.
-   `ipfsHttp` [Object] Setup IPFS HTTP client to be used by ctl.
    -   `ipfsHttp.ref` [Object] Reference to a IPFS HTTP Client object. (defaults to the local require(`ipfs-http-client`))
    -   `ipfsHttp.path` [string] Path to a IPFS HTTP Client to be required. (defaults to the local require.resolve('ipfs-http-client'))
-   `ipfsApi` [Object] Setup IPFS API to be used by ctl.
    -   `ipfsApi.ref` [Object] Reference to a IPFS API object. (defaults to the local require(`ipfs`))
    -   `ipfsApi.path` [string] Path to a IPFS API implementation to be required. (defaults to the local require.resolve('ipfs'))
-   `ipfsBin` [String] Path to a IPFS exectutable . (defaults to the local 'js-ipfs/src/bin/cli.js')
-   `ipfsOptions` [IpfsOptions] Options for the IPFS instance
```

- Previous default ipfs config is only applied when `test` options equals `true`
- `defaultAddrs` option was removed 
- Spawn options are the same as `createFactory`

Old
```
- `options` is an optional object the following properties:
  - `init` bool (default true) or Object - should the node be initialized
  - `initOptions` object - should be of the form `{bits: <size>}`, which sets the desired key size
  - `start` bool (default true) - should the node be started
  - `repoPath` string - the repository path to use for this node, ignored if node is disposable
  - `disposable` bool (default true) - a new repo is created and initialized for each invocation, as well as cleaned up automatically once the process exits
  - `defaultAddrs` bool (default false) - use the daemon default `Swarm` addrs
  - `args` - array of cmd line arguments to be passed to ipfs daemon
  - `config` - ipfs configuration options
```

**NEW**
Same as js-ipfs constructor https://github.com/ipfs/js-ipfs#ipfs-constructor
- ipfsd.killProcess removed not needed anymore
- ipfsd.getConfig removed call ipfsd.api.config.get instead
- ipfsd.setConfig removed, call ipfsd.api.config.set instead

**Read the README for documention on the new api and options**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
exp/wizard Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P0 Critical: Tackled by core team ASAP
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hugomrdias @achingbrain