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.

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

The big refactor! #200

Merged
merged 37 commits into from
Feb 12, 2018
Merged

The big refactor! #200

merged 37 commits into from
Feb 12, 2018

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Feb 7, 2018

The current ipfsd-ctl is a big mess. There are no only a few browser tests; tests have the setup assert and teardown phases all mixed with sometimes using a describe + before as the assertion; there are multiple files that get loaded in the browser without necessity, this is causing all sorts of problems on other modules ipfs-inactive/js-ipfs-http-client#683; The internals use different names for the things that are exposed (i.e calling daemon-ctrl to the Daemon Factory).

There is still a considerable amount of work to do here. I might not be able to finish this today but I intend to have it tomorrow.

@ghost ghost assigned daviddias Feb 7, 2018
@ghost ghost added the status/in-progress In progress label Feb 7, 2018
@dryajov
Copy link
Member

dryajov commented Feb 7, 2018

@diasdavid I'm not sure what you mean by:

There are no browser tests;

All tests run in the browser, what other browser tests are missing?

@daviddias
Copy link
Member Author

I take that back, there is indeed a suite that gets run in the browser https://github.com/ipfs/js-ipfsd-ctl/blob/master/test/browser.js#L4

@dryajov
Copy link
Member

dryajov commented Feb 7, 2018

@diasdavid right, the rest of the tests are concerned with daemon specific tests (i.e. repo exist in the filesystem, daemon cleans up correctly when killed, executable is able to resolve correctly, etc... which can't be tested from the browser).

Thanks for the refactor 👍 I think it cleans up a lot of things.

.aegir.js Outdated
@@ -2,7 +2,8 @@

const createServer = require('./src').createServer

const server = createServer()
const server = createServer({ port: 25000 })
Copy link
Member

Choose a reason for hiding this comment

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

Can we use port 0 to avoid port collisions?

Copy link
Member Author

@daviddias daviddias Feb 9, 2018

Choose a reason for hiding this comment

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

This is spawned in a separate process by aegir, we would have to create yet another side channel just to pass this port over. Not worth it. For other cases, yes for port 0.

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Made good progress this morning, starting to see the light at the end of the tunnel :)

The commit history is a mess because I had to change so many things that at some point it became meaningless.

Still some work TODO though. The project is finally in a state where the structure feels right and it is simple to isolate tests and bugs. I won't be able to touch this again until tomorrow morning. If you have time to contribute, please do :)

| +-----------------------+ | +-----------------------|---- | |
| | | +----------------------+ |
+-----------------------------+ +-----------------------------+
```
Copy link
Member Author

Choose a reason for hiding this comment

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

Needs a new diagram.

Copy link
Member

Choose a reason for hiding this comment

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

What is the current diagram missing?

└── tmp-dir.js

4 directories, 21 files
```
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'm finally happy with the code structure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here for reference:

src
├── defaults
│   ├── config.json
│   └── options.json
├── endpoint                    # endpoint to support remote spawning
│   ├── routes.js
│   └── server.js
├── factory-client.js           # IPFS Factories: client (remote), daemon (go or js) and in-proc (js)
├── factory-daemon.js
├── factory-in-proc.js
├── index.js
├── ipfsd-client.js             # ipfsd (Daemon Controller): client (remote), daemon (go or js), in-proc (js)
├── ipfsd-daemon.js
├── ipfsd-in-proc.js
└── utils                       # Utils used by the Factories and Daemon Controllers
    ├── configure-node.js
    ├── exec.js
    ├── find-ipfs-executable.js
    ├── flatten.js
    ├── parse-config.js
    ├── repo
    │   ├── create-browser.js
    │   └── create-nodejs.js
    ├── run.js
    ├── set-config-value.js
    └── tmp-dir.js

4 directories, 21 files

const ctrl = new CtrlFactory({ type: payload.type })
ctrl.spawn(payload.opts, (err, ipfsd) => {

// TODO: use the ../src/index.js so that the right Factory is picked
Copy link
Member Author

Choose a reason for hiding this comment

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

  • 🌟

}
options = Object.assign({}, options, { type: this.type })
// TODO: (1) this should check to see if it is looking for Go or JS
// TODO: (2) This spawns a whole daemon just to get his version? There is
Copy link
Member Author

Choose a reason for hiding this comment

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

  • 🌟

Copy link
Member

Choose a reason for hiding this comment

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

just doing new Daemon won't actually start the daemon, it will simply allow you to send commands to the selected executable.

Copy link
Member Author

Choose a reason for hiding this comment

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

It still should check if it is JS or Go

Copy link
Member

@dryajov dryajov Feb 10, 2018

Choose a reason for hiding this comment

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

Why? All the logic around daemon type is already being handled in the Daemon itself - not sure what to do with the check here.

callback = options
options = {}
}
// TODO create this method
Copy link
Member Author

@daviddias daviddias Feb 9, 2018

Choose a reason for hiding this comment

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

  • 🌟

@@ -323,7 +327,8 @@ class Node {
),
(config, cb) => {
if (!key) {
return tryJsonParse(config, cb)
// TODO: this function didn't exist on the original codebase
// return tryJsonParse(config, cb)
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Figure out why this was here in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

because the daemon process will return a json string back, not an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dryajov even if that is correct, the tryJsonParse function doesn't seem to exist anywhere in the codebase

expect(ipfsd).to.exist()
})

// TODO fix this test
Copy link
Member Author

Choose a reason for hiding this comment

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

  • 🌟

it('.start with flags', (done) => {
// TODO js-ipfs doesn't fail on unrecognized args. Think what should be
// the desired behaviour
if (type === 'js') { return this.skip() }
Copy link
Member Author

Choose a reason for hiding this comment

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

  • 🌟

@@ -3,44 +3,20 @@
const defaults = require('lodash.defaultsdeep')
Copy link
Member

Choose a reason for hiding this comment

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

why duplicate if factory-in-proc share %90+ code with factory-daemon?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should have a base class then. The main reason is that you want to avoid having factory-daemon being loaded in the browser (and all of its dependencies).

@@ -0,0 +1,63 @@
/* eslint-env mocha */
Copy link
Member

Choose a reason for hiding this comment

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

This was originally intended as a set of reusable tests to run after the different combinations of spawn/init/start to make sure the daemon were being created in a functional way - this intent is lost with this change. How are we testing for this cases?

df = DaemonFactory.create(dfOpts)
})

// TODO: why does ipfsd-ctl polute the env variables?
Copy link
Member

Choose a reason for hiding this comment

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

It used to set IPFS_PATH and other variables, its not the case anymore - we can remove that.

const options = {
init: false,
start: false,
disposable: true
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be false

})

// TODO ?? What is this test trying to prove?
describe('spawn a node and attach api', () => {
Copy link
Member

Choose a reason for hiding this comment

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

confusing name for the test - its trying to make sure that starting the node withspawn with the default options will return a fully constructed ipfsd object. The reusable tests I pointed to above where there to make sure it was also started and initialized correct and permitted to interact with it over the attached ipfs-api.

}
], (err) => {
expect(err).to.not.exist()
expect(fs.existsSync(repoPath)).to.be.ok()
Copy link
Member

Choose a reason for hiding this comment

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

// TODO why wouldn't this work from the browser if we use the
// remote endpoint?

because we use fs.existsSync - is there a way of making it work in the browser?

@ghost ghost assigned dryajov Feb 9, 2018
@dryajov
Copy link
Member

dryajov commented Feb 9, 2018

@diasdavid I took on fixing some of the outstanding todos as well as making sure all tests pass in node.

README.md Outdated

## Table of Contents

- [Install](#install)
- [Usage](#usage)
- [API](#api)
- [Packageing](#packaging)
Copy link
Member Author

@daviddias daviddias Feb 10, 2018

Choose a reason for hiding this comment

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

  • typo

callback = options
options = {}
}
// TODO implement this method
Copy link
Member Author

@daviddias daviddias Feb 10, 2018

Choose a reason for hiding this comment

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

  • This method needs to be implemented as well for clients. Once it is, enable tests again.

Copy link
Member

Choose a reason for hiding this comment

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

In progress, will push shortly.

}
options = Object.assign({}, options, { type: this.type })
// TODO: (1) this should check to see if it is looking for Go or JS
// TODO: (2) This spawns a whole daemon just to get his version? There is
Copy link
Member Author

Choose a reason for hiding this comment

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

It still should check if it is JS or Go

@daviddias
Copy link
Member Author

Ok, fixed some of the tests, now I'm seeing a new issue. It seems that when spawning a js-ipfs daemon and/or in-proc node, it doesn't respect the default config of ipfsd-ctl and uses the default of js-ipfs causing port clashes

image

const nonDisposableConfig = clone(defaultConfig)
delete nonDisposableConfig.Addresses
// TODO why delete the addrs here???
// delete nonDisposableConfig.Addresses
Copy link
Member Author

Choose a reason for hiding this comment

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

@dryajov why were you deleting the addrs here?

Copy link
Member

Choose a reason for hiding this comment

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

Because at some point we wanted non-disposable nodes to use js-ipfs or go-ipfs default ports and not random ports.

@@ -26,7 +26,7 @@ class FactoryDaemon {
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)
options = Object.assign({}, { type: 'go' }, options)
Copy link
Member Author

Choose a reason for hiding this comment

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

@dryajov why this change?

@@ -4,11 +4,14 @@ const defaults = require('lodash.defaultsdeep')
const clone = require('lodash.clone')
const waterfall = require('async/waterfall')
const path = require('path')
const tmpDir = require('./utils/tmp-dir')
Copy link
Member Author

Choose a reason for hiding this comment

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

This brought the os module to the browser.

@daviddias
Copy link
Member Author

daviddias commented Feb 12, 2018

image

Woot! Good to merge. Deferring remaining tune up to later PRs.

Also tested with js-ipfs-api and ipfs/interop

@daviddias daviddias merged commit adbef1b into master Feb 12, 2018
@daviddias daviddias deleted the ipfsd-light branch February 12, 2018 12:01
@ghost ghost removed the status/in-progress In progress label Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants