Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

files add, cat, get core + cli offline #193

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,46 +37,47 @@
},
"homepage": "https://github.com/ipfs/js-ipfs#readme",
"devDependencies": {
"aegir": "^3.0.0",
"aegir": "^3.0.1",
"async": "^2.0.0-rc.3",
"buffer-loader": "0.0.1",
"chai": "^3.5.0",
"expose-loader": "^0.7.1",
"form-data": "^1.0.0-rc3",
"idb-plus-blob-store": "^1.1.2",
"lodash": "^4.11.1",
"mocha": "^2.3.4",
"lodash": "^4.11.2",
"mocha": "^2.4.5",
"ncp": "^2.0.0",
"nexpect": "^0.5.0",
"pre-commit": "^1.1.2",
"rimraf": "^2.4.4",
"rimraf": "^2.5.2",
"stream-to-promise": "^1.1.0",
"transform-loader": "^0.2.3"
},
"dependencies": {
"babel-runtime": "^6.6.1",
"bl": "^1.1.2",
"boom": "^3.1.1",
"boom": "^3.1.2",
"bs58": "^3.0.0",
"debug": "^2.2.0",
"fs-blob-store": "^5.2.1",
"glob": "^7.0.3",
"hapi": "^13.3.0",
"ipfs-api": "^3.0.1",
"ipfs-api": "^3.0.2",
"ipfs-block": "^0.3.0",
"ipfs-block-service": "^0.3.0",
"ipfs-data-importing": "^0.3.3",
"ipfs-merkle-dag": "^0.5.0",
"ipfs-multipart": "^0.1.0",
"ipfs-repo": "^0.8.0",
"joi": "^8.0.2",
"libp2p-ipfs": "^0.3.3",
"ipfs-unixfs-engine": "^0.6.1",
"joi": "^8.0.5",
"libp2p-ipfs": "^0.3.5",
"lodash.get": "^4.2.1",
"lodash.set": "^4.0.0",
"multiaddr": "^1.3.0",
"lodash.set": "^4.1.0",
"multiaddr": "^1.4.1",
"peer-book": "0.1.0",
"peer-id": "^0.6.6",
"peer-info": "^0.6.2",
"readable-stream": "^1.1.13",
"ronin": "^0.3.11",
"temp": "^0.8.3"
},
Expand Down
77 changes: 69 additions & 8 deletions src/cli/commands/files/add.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
'use strict'

const Command = require('ronin').Command
const IPFS = require('../../../core')
const utils = require('../../utils')
const debug = require('debug')
const log = debug('cli:version')
log.error = debug('cli:version:error')
const bs58 = require('bs58')
const Readable = require('stream').Readable
const fs = require('fs')
const async = require('async')
const pathj = require('path')
Copy link
Member

Choose a reason for hiding this comment

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

use path for the name of the variable that contains the module, and use another keyword for the argument path. So that we keep the usage of the module consistent

const glob = require('glob')

module.exports = Command.extend({
desc: 'Add a file to IPFS using the UnixFS data format',
Expand All @@ -19,15 +24,71 @@ module.exports = Command.extend({
},

run: (recursive, path) => {
var node = new IPFS()
path = process.cwd() + '/' + path
node.files.add(path, {
recursive: recursive
}, (err, stats) => {
let rs

if (!path) {
throw new Error('Error: Argument \'path\' is required')
}

var s = fs.statSync(path)

if (s.isDirectory() && recursive === false) {
throw new Error('Error: ' + process.cwd() + ' is a directory, use the \'-r\' flag to specify directories')
}
if (path === '.' && recursive === true) {
path = process.cwd()
s = fs.statSync(process.cwd())
} else if (path === '.' && recursive === false) {
s = fs.statSync(process.cwd())
if (s.isDirectory()) {
throw new Error('Error: ' + process.cwd() + ' is a directory, use the \'-r\' flag to specify directories')
}
}
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 goal of this chunk of code? Can it be extracted to a function with a good name? At first glance seems madness to me :D

Copy link
Member Author

@nginnever nginnever May 5, 2016

Choose a reason for hiding this comment

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

It's to check for all of the inputs you could give to add command. variations i could think of are...

  1. "." add the cwd but throw error for no recursion flag
  2. "." -r add the cwd
  3. "/some/path" but throw error for no recursion
  4. "/some/path" -r
  5. No path, throw err
  6. filename.type

I could make a checkpath() function but i'm not sure how that would simplify any of the logic

Copy link
Member

Choose a reason for hiding this comment

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

Creating a function and adding that as a comment describing what it is doing would be considerably much better.


glob(pathj.join(path, '/**/*'), (err, res) => {
if (err) {
return console.log(err)
throw err
}
console.log('added', bs58.encode(stats.Hash).toString(), stats.Name)
utils.getIPFS((err, ipfs) => {
if (err) {
throw err
}
const i = ipfs.files.add()
i.on('data', (file) => {
console.log('added', bs58.encode(file.multihash).toString(), file.path)
})
Copy link
Member

Choose a reason for hiding this comment

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

You never listen for the end event, how can you tell it is complete?

Copy link
Member Author

@nginnever nginnever May 5, 2016

Choose a reason for hiding this comment

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

What would I need to do when it is complete? It's just printing the responses until there is nothing left to print

Copy link
Member

Choose a reason for hiding this comment

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

The process should exit gracefully

if (res.length !== 0) {
const index = path.lastIndexOf('/')
async.eachLimit(res, 10, (element, callback) => {
rs = new Readable()
Copy link
Member

Choose a reason for hiding this comment

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

You can declare rs in here, no need for it at the outer scope

Copy link
Member Author

Choose a reason for hiding this comment

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

80:11 error 'rs' is not defined no-undef
83:11 error 'rs' is not defined no-undef
84:11 error 'rs' is not defined no-undef
85:49 error 'rs' is not defined

Copy link
Member

Choose a reason for hiding this comment

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

yeah you will have to define it again on line 83 here, remember let and const are block scoped

Copy link
Member Author

@nginnever nginnever May 5, 2016

Choose a reason for hiding this comment

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

doing that will give a lint error about defining twice, the only reason i moved those decelerations to the top was to avoid lint errors

const addPath = element.substring(index + 1, element.length)
if (fs.statSync(element).isDirectory()) {
callback()
} else {
const buffered = fs.readFileSync(element)
rs.push(buffered)
rs.push(null)
const filePair = {path: addPath, stream: rs}
i.write(filePair)
callback()
}
}, (err) => {
if (err) {
throw err
}
i.end()
})
} else {
rs = new Readable()
Copy link
Member

Choose a reason for hiding this comment

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

these 'forced' reuse of variables feels weird. A variable named should be reused if several points in the code will look at it and react to its change. If you instantiate a new object, but then have code that treats that object specifically, no need to reuse variables. (remember that in JS, each time you reassign a variable, you give V8 an headache :) )

Copy link
Member Author

@nginnever nginnever May 5, 2016

Choose a reason for hiding this comment

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

I don't fully understand what you mean. Will try to ponder on this a bit. The decelerations come in different parts of the logic. Or there shouldn't be a time when the if statement is satisfied for both conditions meaning that it is necessary to instantiate 'rs' in both logic choices. Let me know if I am missing something here.

If you are talking about const buffered and const filepair then i agree that those should be declared above the loop and reassigned

const buffered = fs.readFileSync(path)
Copy link
Member

Choose a reason for hiding this comment

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

Why reading the file all together and shuve it on a readable stream, when you can fs.createReadStream and leverage the backpressured offered by the OS?

path = path.substring(path.lastIndexOf('/') + 1, path.length)
rs.push(buffered)
rs.push(null)
const filePair = {path: path, stream: rs}
i.write(filePair)
i.end()
}
})
})
}
})
37 changes: 37 additions & 0 deletions src/cli/commands/files/cat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict'

const Command = require('ronin').Command
const debug = require('debug')
const utils = require('../../utils')
const log = debug('cli:files')
log.error = debug('cli:files:error')

module.exports = Command.extend({
desc: 'Download IPFS objects',

options: {},

run: (path, options) => {
if (!path) {
throw new Error("Argument 'path' is required")
}
if (!options) {
options = {}
}
utils.getIPFS((err, ipfs) => {
if (err) {
throw err
}
ipfs.files.cat(path, (err, res) => {
if (err) {
throw (err)
}
if (res) {
res.on('file', (data) => {
data.stream.pipe(process.stdout)
})
}
Copy link
Member

Choose a reason for hiding this comment

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

What if not err nor res?

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 will fail silently? not sure what you want me to do here

Copy link
Member Author

Choose a reason for hiding this comment

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

are return statements the proper way to exit gracefully?

})
})
}
})
69 changes: 69 additions & 0 deletions src/cli/commands/files/get.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
'use strict'

const Command = require('ronin').Command
const debug = require('debug')
const utils = require('../../utils')
const log = debug('cli:files')
log.error = debug('cli:files:error')
var fs = require('fs')
const pathj = require('path')

module.exports = Command.extend({
desc: 'Download IPFS objects',

options: {},

run: (path, options) => {
let dir
let filepath
let ws

if (!path) {
throw new Error("Argument 'path' is required")
}
if (!options) {
options = {}
dir = process.cwd()
} else {
if (options.slice(-1) !== '/') {
options += '/'
}
dir = options
Copy link
Member

Choose a reason for hiding this comment

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

if options is the outputPath, shouldn't it be called that?

}

utils.getIPFS((err, ipfs) => {
if (err) {
throw err
}
ipfs.files.get(path, (err, data) => {
if (err) {
throw err
}
data.on('file', (data) => {
if (data.path.lastIndexOf('/') === -1) {
filepath = data.path
Copy link
Member

Choose a reason for hiding this comment

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

only needed in here, no need to be declared at the top level

Copy link
Member Author

Choose a reason for hiding this comment

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

linter:
55:13 error 'filepath' used outside of binding context block-scoped-var
57:44 error 'filepath' used outside of binding context block-scoped-var

if (data.dir === false) {
ws = fs.createWriteStream(pathj.join(dir, data.path))
Copy link
Member

Choose a reason for hiding this comment

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

only needed in here, no need to be declared at the top level

Copy link
Member Author

Choose a reason for hiding this comment

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

60:13 error 'ws' used outside of binding context block-scoped-var
61:30 error 'ws' used outside of binding context block-scoped-var

Copy link
Member

Choose a reason for hiding this comment

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

one scope up sorry,

data.stream.pipe(ws)
} else {
try {
fs.mkdirSync(pathj.join(dir, data.path))
} catch (err) {
throw err
}
}
} else {
filepath = data.path.substring(0, data.path.lastIndexOf('/') + 1)
try {
fs.mkdirSync(pathj.join(dir, filepath))
} catch (err) {
throw err
}
ws = fs.createWriteStream(pathj.join(dir, data.path))
data.stream.pipe(ws)
}
})
})
})
}
})
Loading