feat: allows a custom IpfsApi constructor to be used #261
Conversation
ghost
assigned
|
|
||
| const node = new Daemon(options) | ||
|
|
||
| series([ | ||
| (cb) => options.init | ||
| ? node.init(cb) | ||
| ? node.init(options.initOptions, cb) |
There was a problem hiding this comment.
I believe this was a bug where initOptions weren't being passed to the daemon when they were set at the factory level.
src/factory-client.js
Outdated
| @@ -16,6 +16,7 @@ class FactoryClient { | |||
| options.host = 'localhost' | |||
| } | |||
|
|
|||
| this.options = options | |||
| this.port = options.port | |||
| this.host = options.host | |||
There was a problem hiding this comment.
Since we are storing options in the factory instance, I think we should remove the following two lines (this.port and this.host). In addition, we should change L:28 to:
this.baseUrl = `${options.secure ? 'https://' : 'http://'}${options.host}:${options.port}`This way, we would have a more similar factory with the factory-daemon and the factory-in-proc, which do not have port and host defined in their instances. What do you think?
|
|
||
| const node = new Daemon(options) | ||
|
|
||
| series([ | ||
| (cb) => options.init | ||
| ? node.init(cb) | ||
| ? node.init(options.initOptions, cb) |
src/factory-daemon.js
Outdated
| * @return {*} | ||
| */ | ||
| constructor (options) { | ||
| if (options && options.type === 'proc') { | ||
| throw new Error('This Factory does not know how to spawn in proc nodes') | ||
| } | ||
| options = Object.assign({}, { type: 'go' }, options) | ||
| this.options = options | ||
| this.type = options.type |
There was a problem hiding this comment.
I am thinking if we should also remove this.type, as it is also duplicated.
|
@vasco-santos I moved This is a breaking change if anyone was using these properties directly from the factory so I've labelled the commit as such. |
|
thanks @alanshaw ! I will just wait for the CI Green PR to be merged, and I will merge this afterword. |
vasco-santos
force-pushed
the
feat/pass-your-own-ipfs-api
branch
from
17e355e
to
a7ccdcb
Compare
|
I rebased this with Master and there is a test failing for windows. I will create a new release for |
|
Any news on this? I think the windows test failure is unrelated... |
vasco-santos
force-pushed
the
feat/pass-your-own-ipfs-api
branch
2 times, most recently
from
88b0bc8
to
7ba3238
Compare
|
Hey @alanshaw I have tried to understand this error, but no luck so far. The error appears in the test added in this PR, but I believe it may not be related with the added code. There are some reports online regarding the |
License: MIT Signed-off-by: Alan Shaw <[email protected]>
BREAKING CHANGE: host, port, type and exec are no longer accessible directly from factory instances, use `options.type` etc. instead. License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
alanshaw
force-pushed
the
feat/pass-your-own-ipfs-api
branch
from
7ba3238
to
f17ee4b
Compare
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
|
@vasco-santos the issue was that the test was relying on ipfsd-ctl to kill the process using SIGKILL after it didn't shutdown gracefully. This would have been fine except that windows doesn't support SIGKILL and fails with the I've changed the PR to use a proper API client to stop the node using the Hopefully this is good to merge now? |
We decided in #257 that it would be best to bundle ipfs, ipfs-api and go-ipfs-dep but allow the user to pass their own implementations for "advanced mode" usage aka. us and our tests.
This PR is step 1 and it allows the user to pass an
IpfsApiconstructor toFactory.createorf.spawn.