Multidat #198

yoshuawuyts · 2017-02-04T09:22:42Z

This touches some of the same code as #173. I think once this patch is merged we can move to some of the ergonomics #173 provides (e.g. using getters and the like for state access). This patch should provide us with some robust underpinnings to work on top ✨

changes

removes all dependencies on level which preps us for the upcoming SLEEP release
moved state into ~/.dat-desktop-db.json using toiletdb
renamed some methods on model/repos to be more in line with the dat cli
moved our custom dat management code into multidat library. It's fully tested to make sure it works as expected and relies on multidrive and the latest dat-node
made all unhandled errors call back instead. Might lead to crashes where errors were previously swallowed
added a test to catch unused deps
removed a whole bunch of unused deps
removed more code than was added which is always 🆒

todo

use the multidat.readManifest() method to determine author and title
fix key restart regression in dat-ecosystem-archive/dat-node@508de80

notes

placing this under the public launch milestone because moving off level should be done sooner rather than later
I thought Upgrade to dat-node@1. Fixes #168 #173 was merged already before submitting this PR; apologies that there's some overlap there!
during the writing of this patch we found a few subtle bugs in dat-node; they're solved now but we'll probably run into more
there's less DB lock errors it seems, but we're hitting different state corruptions now; I think we'll need to focus on debugging for the next week or so to have it stable enough for a release

juliangruber

..otherwise, NIIIICE!

juliangruber · 2017-02-04T18:01:24Z

lib/root-dir.js

-const fs = require('fs')
-
-const argv = minimist(remoteProcess.argv.slice(2))
-const rootDir = argv.data || `${app.getPath('downloads')}/dat`


can we add this back? this is required so you can run two instances of dat-desktop on the same machine, to verify they're able to replicate with each other properly

oh yeah sounds good - we should probably come up with a better way to handle options later, but re-adding ✨

the mkdirSync is missing still. why not just add back this file as a whole?

juliangruber · 2017-02-04T18:02:10Z

models/repos.js

+    var dblocation = path.join(process.env.HOME)
+    var dbfile = path.join(dblocation, '.dat-desktop-db.json')
+
+    var arr = [


this variable name is a bit generic, can we change this to waterfall([fun... or var tasks?

oh yeah, I like tasks!

juliangruber · 2017-02-04T18:02:26Z

models/repos.js

-    }), 1500, true)
+  // boot multidat, create the ~/Downloads/dat directory
+  function startMultidat (send, done) {
+    var dblocation = path.join(process.env.HOME)


should be camelCased, to be consistent

juliangruber · 2017-02-04T18:03:38Z

models/repos.js

+      function (next) {
+        mkdirp(dblocation, next)
+      },
+      function (val, next) {


we're not using val, can you rename it to _?
also the val below would be more clear when renamed to multidat, as that's the identifier used below.

juliangruber · 2017-02-04T18:05:26Z

models/repos.js

+  function removeDat (state, data, send, done) {
+    assert.ok(data.key, 'repos-model.deleteDat: data.key should exist')
+
+    if (data.confirmed === true) {


because the case where data.confirmed === false always happens first, can we list it first here too?

ah yeah, nice one (:

juliangruber · 2017-02-04T18:05:50Z

models/repos.js

+  }
+
+  function cloneDat (state, data, send, done) {
+    assert.ok(Buffer.isBuffer(data) || typeof data === 'string', 'repos-model.cloneDat: data should be a buffer or a string')


this is taken care of by dat-encoding

not necessarily - if undefined is passed for example we'll get a weird error somewhere down in a package - this guards against that

(hope you ok with this? - I know I use assert a lot, but feel we're still at a spot where we regularly get weird errors that having lots of asserts can help us catch and trace bugs better)

dat-encoding has sufficient guards now too, see https://github.com/juliangruber/dat-encoding/blob/master/index.js, but i'm also ok with leaving this here, for extra clarity

juliangruber · 2017-02-04T18:06:07Z

models/repos.js

+  function cloneDat (state, data, send, done) {
+    assert.ok(Buffer.isBuffer(data) || typeof data === 'string', 'repos-model.cloneDat: data should be a buffer or a string')
+
+    var key = encoding.decode(data).toString('hex')


can be refactored to var key = encoding.toStr(data)

juliangruber · 2017-02-04T18:08:02Z

package.json

@@ -7,45 +7,37 @@
  "description": "Dat Desktop App",
  "author": "Dat Team",
  "dependencies": {
-    "aliasify": "^2.1.0",


thanks for cleaning this up

yoshuawuyts · 2017-02-04T20:31:06Z

@juliangruber doneeeeee ✨

juliangruber · 2017-02-05T11:04:28Z

models/repos.js

@@ -195,6 +195,14 @@ function createManager (multidat, onupdate) {
    var stats = dat.trackStats()
    dat.stats = stats

+    multidat.readManifest(dat, function (_, manifest) {


looks like this fn is only called once, however the metadata should update whenever the dat.json changes

juliangruber · 2017-02-08T07:40:19Z

elements/table.js

@@ -196,12 +194,15 @@ function tableElement (dats, send) {
 // ([obj], fn) -> html
 function createTable (dats, send) {
  return dats.map(dat => {
-    const progress = dat.stats.blocksTotal
-      ? Math.round(dat.stats.blocksProgress / dat.stats.blocksTotal * 100)
+    const stats = dat.stats && dat.stats.get()


shouldn't dat.stats always be set? also, this getter was more convenient to use: https://github.com/datproject/dat-desktop/pull/198/files#diff-66caf64acff533dbfc525b13489309cdL178

not a fan of getters / setters / proxies as an API b/c it hides what's going on; but yeah think you're right - it's set always now; was being somewhat defensive (:

yeah, there's a balance with defensive programming and it's not easy to determine. however since we know there's always dat.stats, it's clearer this way. Otherwise you can read the code above and think otherwise.

Not programming defensively also speaks.

juliangruber · 2017-02-08T07:41:00Z

elements/table.js

-    const progress = dat.stats.blocksTotal
-      ? Math.round(dat.stats.blocksProgress / dat.stats.blocksTotal * 100)
+    const stats = dat.stats && dat.stats.get()
+    const progress = (stats)


why are you adding parens around single values? precedence over the identity is always true

const progress = stats looks like an assignment without parens imo; always write my ternaries / arrow functions with parens; helps with readability

const progress = (stats) can be an assignment too though :P

My main point with style is just that it should be consistent over the whole project. Either stick to the existing style, or make the rest of the code base follow along in an atomic step.

juliangruber · 2017-02-08T07:42:19Z

lib/root-dir.js

-const fs = require('fs')
-
-const argv = minimist(remoteProcess.argv.slice(2))
-const rootDir = argv.data || `${app.getPath('downloads')}/dat`


the mkdirSync is missing still. why not just add back this file as a whole?

juliangruber · 2017-02-08T07:42:34Z

models/repos.js

-    })
-  })
+function createModel () {
+  var manager = null


juliangruber · 2017-02-08T07:43:14Z

models/repos.js

+
+    var tasks = [
+      function (next) {
+        mkdirp(dbLocation, next)


process.env.HOME always exists, no need to mkdirp it

juliangruber · 2017-02-08T07:43:29Z

models/repos.js

+        mkdirp(dbLocation, next)
+      },
+      function (_, next) {
+        var db = toilet(dbFile)


juliangruber · 2017-02-08T07:49:24Z

models/repos.js

+  }
+
+  function cloneDat (state, data, send, done) {
+    assert.ok(Buffer.isBuffer(data) || typeof data === 'string', 'repos-model.cloneDat: data should be a buffer or a string')


dat-encoding has sufficient guards now too, see https://github.com/juliangruber/dat-encoding/blob/master/index.js, but i'm also ok with leaving this here, for extra clarity

juliangruber · 2017-02-08T07:50:35Z

pages/main-delete.js

-      ${confirmModal(() => send('repos:deleteConfirm', link))}
+      ${confirmModal(() => send('repos:remove', {
+        confirmed: true,
+        key: link


having key and link makes this confusing, do you think we can get it to just use one term across the codebase?

👍 I'm guilty of this too in the CLI.

In my mind key the 64 character hash (mostly as a buffer), without any other characters. link is any think that contains a key (or references one), e.g. dat://<key>, datproject.org/view/<key>, datproject.org/karissa/more-tweets-more-votes (which resolves a key via API). Though I mostly refer to all of these as key in the CLI.

But we should decide on a consistent terminology.

@juliangruber

fixup! more stuff fixup! fix bugggssss fixup! finish like most of this stuff fixup! rm unused deps fixup! default progress to 0 setup metadata add @juliangruber's feedback

yoshuawuyts · 2017-02-09T11:24:11Z

just arrived at a place with internet so pushed final changes based on comments you made; tried pushing yest but somehow github wouldn't let me. Should be all good now I think - feel free to merge.

Big warning on that this might cause all sorts of bugs to pop up b/c we're handling more errors now; if you folks could go ahead and test things that'd be very valuable I reckon. Cheers!

juliangruber · 2017-02-09T13:17:37Z

you force pushed over commits i added to this branch, that's why github didn't let you

yoshuawuyts force-pushed the multidat branch from e20a486 to 3903821 Compare February 4, 2017 09:23

yoshuawuyts requested a review from juliangruber February 4, 2017 09:23

yoshuawuyts self-assigned this Feb 4, 2017

yoshuawuyts added this to the Public Launch milestone Feb 4, 2017

juliangruber suggested changes Feb 4, 2017

View reviewed changes

juliangruber suggested changes Feb 5, 2017

View reviewed changes

yoshuawuyts force-pushed the multidat branch 2 times, most recently from c7e8631 to aa9e16f Compare February 8, 2017 04:06

juliangruber suggested changes Feb 8, 2017

View reviewed changes

use multidat

2afa2f9

fixup! more stuff fixup! fix bugggssss fixup! finish like most of this stuff fixup! rm unused deps fixup! default progress to 0 setup metadata add @juliangruber's feedback

juliangruber approved these changes Feb 9, 2017

View reviewed changes

yoshuawuyts force-pushed the multidat branch from 5f83534 to 2afa2f9 Compare February 9, 2017 11:22

yoshuawuyts merged commit 2754471 into master Feb 10, 2017

yoshuawuyts deleted the multidat branch February 10, 2017 08:52

yoshuawuyts mentioned this pull request Feb 10, 2017

Include meta in total progress. Closes #166 #174

Closed

Multidat #198

Multidat #198

Conversation

yoshuawuyts commented Feb 4, 2017 • edited by joehand Loading

changes

todo

notes

juliangruber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliangruber Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Feb 9, 2017

juliangruber commented Feb 9, 2017

yoshuawuyts commented Feb 4, 2017 •

edited by joehand

Loading

juliangruber Feb 8, 2017 •

edited

Loading