feat: better stream handling #140
Conversation
- upgrade aegir - upgrade dependencies - handle stream errors when setting the config - use pump instead of pipe
| "mkdirp": "^0.5.1", | ||
| "multihashes": "^0.3.1", |
There was a problem hiding this comment.
Is multihashes needed somewhere else? I don't see it being used in the tests.
There was a problem hiding this comment.
|
One comment and once CI is happy, LGTM. |
Thanks to the great work from @JGAntunes
| "go-ipfs-dep": "0.4.4", | ||
| "ipfs-api": "^12.0.3", | ||
| "multiaddr": "^2.1.0", | ||
| "once": "^1.4.0", | ||
| "rimraf": "^2.5.4", | ||
| "run-series": "^1.1.4", | ||
| "shutdown": "^0.2.4", | ||
| "subcomandante": "^1.0.5" |
There was a problem hiding this comment.
@jbenet invested a lot of time making sure we had a thing that would not leave zombie IPFS processes in user machines. It was what blocked #89 all this time.
Until we have comprehensive tests to make sure that you can spawn a bunch of daemons and make sure they don't become zombies, we can't move to execa. Historically, a way to test this was with station
If I understand correctly, migrating to execa is not a requirement for any particular feature.
There was a problem hiding this comment.
I don't see how the burden of proof lies on the one changing it, as I haven't seen any proof subcomandante being as good as you say it is. There are tests for the cleanup behavior inside execa, which I wrote when implementing. I also did manual tests with execa inside reusing the testsuite from subcomandante so those cases all work.
So far all I heard was 'there were extensive tests' but I haven't seen or read what those actually were, so please tell which scenarios you are concerned about and I can add tests for them
There was a problem hiding this comment.
@diasdavid I've ported the test suite from node-subcomandante, to run against the new src/exec.js to add a baseline of coverage to this module.
There was a problem hiding this comment.
Soo even though those tests pass fine, it seems thing are wonky right now. I have 5 ipfs instances running after a single npm test run :(
I will revert the switch to execa tomorrow.
| disposable: true, | ||
| init: true | ||
| } | ||
|
|
||
| function tempDir () { | ||
| return join(os.tmpdir(), `ipfs_${String(Math.random()).substr(2)}`) | ||
| } |
There was a problem hiding this comment.
Is the path to tmpDir available through some call?
| @@ -64,25 +83,20 @@ module.exports = class Node { | |||
| this.initialized = fs.existsSync(path) | |||
| this.clean = true | |||
| this.env = Object.assign({}, process.env, {IPFS_PATH: path}) | |||
| this.disposable = disposable | |||
|
|
|||
There was a problem hiding this comment.
add to the list of props path as the path used for the repo
There was a problem hiding this comment.
not sure, I didn't change anything from that logic
There was a problem hiding this comment.
this comment was meant for the one above
There was a problem hiding this comment.
add to the list of props path as the path used for the repo
already there if you look at the full file
| if (err) throw err | ||
| done() | ||
| }) | ||
| shutdown (callback) { |
There was a problem hiding this comment.
still a bad name, check the deleted comment
There was a problem hiding this comment.
but independent of the name, it should be document of what it is doing
There was a problem hiding this comment.
I disagree I think it's actuallly a pretty fitting name, in terms of docs nothing changed
There was a problem hiding this comment.
There are no docs (am I missing something?)
There was a problem hiding this comment.
There is a comment above which kinda describes what it does, no other docs currently, correct
|
Added documentation https://ipfs.github.io/js-ipfsd-ctl/ |
|
Looks like I can't run the last test on travis: |
|
@diasdavid this should be ready now, we are back to subcomandante and have more tests and docs |
I've also pulled in the changes from subcomandante to execa from #89