-
Notifications
You must be signed in to change notification settings - Fork 61
fix: only require http api client if it has not been specified #450
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.
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
Changes from 16 commits
3a0f870
ad2e60d
1fd7dcb
da43d03
f010017
43e01d8
13ced9c
2fe63c2
784bccd
3d813a0
21bd28f
5d77e03
be018b5
769d624
21fc98e
f92aaf2
e46948e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,11 +21,6 @@ const defaults = { | |
| type: 'go', | ||
| env: {}, | ||
| args: [], | ||
| ipfsHttpModule: { | ||
| path: require.resolve('ipfs-http-client'), | ||
| ref: require('ipfs-http-client') | ||
| }, | ||
| ipfsModule: {}, | ||
| ipfsOptions: {}, | ||
| forceKill: true, | ||
| forceKillTimeout: 5000 | ||
|
|
@@ -51,14 +46,6 @@ class Factory { | |
| proc: merge(this.opts, { type: 'proc' }) | ||
| }, overrides) | ||
|
|
||
| if (!this.overrides.js.ipfsBin) { | ||
| this.overrides.js.ipfsBin = findBin('js', this.opts.type === 'js') | ||
| } | ||
|
|
||
| if (!this.overrides.go.ipfsBin) { | ||
| this.overrides.go.ipfsBin = findBin('go', this.opts.type === 'go') | ||
| } | ||
|
|
||
| /** @type ControllerDaemon[] */ | ||
| this.controllers = [] | ||
| } | ||
|
|
@@ -87,18 +74,39 @@ class Factory { | |
| } | ||
|
|
||
| async _spawnRemote (options) { | ||
| const res = await ky.post( | ||
| `${options.endpoint}/spawn`, | ||
| { | ||
| json: { | ||
| ...options, | ||
| // avoid recursive spawning | ||
| remote: false, | ||
| // do not send code refs over http | ||
| ipfsModule: { ...options.ipfsModule, ref: undefined }, | ||
| ipfsHttpModule: { ...options.ipfsHttpModule, ref: undefined } | ||
| const opts = { | ||
| json: { | ||
| ...options, | ||
| // avoid recursive spawning | ||
| remote: false | ||
| } | ||
| } | ||
|
|
||
| if (options.ipfsModule) { | ||
| delete opts.ipfsModule | ||
|
|
||
| if (options.ipfsModule.path) { | ||
| opts.ipfsModule = { | ||
| path: options.ipfsModule.path | ||
| // n.b. no ref property - do not send code refs over http | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (options.ipfsHttpModule) { | ||
| delete opts.ipfsHttpModule | ||
|
|
||
| if (options.ipfsHttpModule.path) { | ||
| opts.ipfsHttpModule = { | ||
| path: options.ipfsHttpModule.path | ||
| // n.b. no ref property - do not send code refs over http | ||
| } | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need all this haven't we already done this below? we just need to remove refs like in the previous version of this block. Plus its deleting and setting opts.ipfsModule where it should be opts.json.ipfsModule and same for opts.ipfsHttpModule, so if this code made some use case work it wasn't because of this logic.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was resulting in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these if clauses are very hard to track and error prone because of all the combinations available in ctl
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point about the deletion of the wrong property though.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You are quite right. Here, look - I've removed them: #454 |
||
|
|
||
| const res = await ky.post( | ||
| `${options.endpoint}/spawn`, | ||
| opts | ||
| ).json() | ||
| return new ControllerRemote( | ||
| options.endpoint, | ||
|
|
@@ -116,16 +124,39 @@ class Factory { | |
| const type = options.type || this.opts.type | ||
| const opts = merge( | ||
| this.overrides[type], | ||
| // conditionally include ipfs based on which type of daemon we will spawn when none has been specifed | ||
| (type === 'js' || type === 'proc') ? { | ||
| ipfsModule: { | ||
| path: require.resolve('ipfs'), | ||
| ref: require('ipfs') | ||
| } | ||
| } : {}, | ||
| options | ||
| ) | ||
|
|
||
| // conditionally include ipfs based on which type of daemon we will spawn when none has been specified | ||
| if ((opts.type === 'js' || opts.type === 'proc') && !opts.ipfsModule) { | ||
| opts.ipfsModule = {} | ||
| } | ||
|
|
||
| if (opts.ipfsModule) { | ||
| if (!opts.ipfsModule.path) { | ||
| opts.ipfsModule.path = require.resolve('ipfs') | ||
| } | ||
|
|
||
| if (!opts.ipfsModule.ref) { | ||
| opts.ipfsModule.ref = require('ipfs') | ||
| } | ||
| } | ||
|
|
||
| // only include the http api client if it has not been specified as an option | ||
| // for example if we are testing the http api client itself we should not try | ||
| // to require 'ipfs-http-client' | ||
| if (!opts.ipfsHttpModule) { | ||
| opts.ipfsHttpModule = { | ||
| path: require.resolve('ipfs-http-client'), | ||
| ref: require('ipfs-http-client') | ||
| } | ||
| } | ||
|
|
||
| // find ipfs binary if not specified | ||
| if (opts.type !== 'proc' && !opts.ipfsBin) { | ||
| opts.ipfsBin = findBin(opts.type, true) | ||
| } | ||
|
|
||
| // IPFS options defaults | ||
| const ipfsOptions = merge( | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.