-
Notifications
You must be signed in to change notification settings - Fork 50
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
Datastore integration #123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- remove yarn.lock
README.md
Outdated
│ │ │ │ │ │ │ │ | ||
├─────────────────┤ ├─────────────────┤ ├─────────────────┤ ├─────────────────┤ | ||
│ FlatfsDatastore │ │LevelDBDatastore │ │LevelJSDatastore │ │LevelJSDatastore │ | ||
└─────────────────┘ └─────────────────┘ └─────────────────┘ └─────────────────┘ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drawing missing all the other things.
PS: monodraw is awesome, isn't it? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a drawing of the datastores and how the are used, what you had before were not actual stores
|
||
- The datastore folder holds the legacy version of datastore, still built in levelDB, there is a current endeavour of pushing it to fs completely. | ||
- The blocks folder is the current version of datastore. | ||
- The keys repo doesn't exist yet, as the private key is simply stored inside config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, not important anymore? I think this plus my comment on @SidHarder PR -- #111 (comment) -- are valuable to keep around, we have explained this a countless number of times, we want to avoid spending all of that time again as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not correct anymore, that's why I removed it for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the last item is not valid anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first item is also not true, datastore is not legacy, it's being actively used and things added to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which makes the second incorrect as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making the last item the only one valid
README.md
Outdated
var repo = new IPFSRepo('/Users/someone/.ipfs', { | ||
stores: inMemoryBS | ||
const Repo = require('ipfs-repo') | ||
const repo = new Repo('/Users/awesome/.jsipfs') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use the universal /tmp
folder instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
README.md
Outdated
stores: inMemoryBS | ||
const Repo = require('ipfs-repo') | ||
const repo = new Repo('/Users/awesome/.jsipfs') | ||
repo.init(defaultConfig, (err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an example of what is the defaultConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
README.md
Outdated
} | ||
|
||
console.log('repo is ready') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tree
of the folder created would be useful here, just to give a visual representation of what is the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
package.json
Outdated
"browser": { | ||
"./src/lock.js": "./src/lock-browser.js", | ||
"./src/default-config.js": "./src/default-config-browser.js" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was weird to me, there is the 'default-config' object that is passed on init, which apparently is what an IPFS config looks like, but then checking the 'default-config' files, it shows something entirely different.
Remember the lesson of: #111 (comment) :)
Lesson of the story: Naming things is hard, but it is even harder if you give the same name to everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point, any thoughts on better naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to default-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you :)
package.json
Outdated
@@ -31,27 +38,29 @@ | |||
"node": ">=4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are it, add npm engine as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -31,27 +38,29 @@ | |||
"node": ">=4.0.0" | |||
}, | |||
"devDependencies": { | |||
"aegir": "^10.0.0", | |||
"buffer-loader": "^0.0.1", | |||
"aegir": "^11.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
*/ | ||
close (callback) { | ||
if (this.closed) { | ||
return callback(new Error('repo is already closed')) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different behaviour. open will return silently if it is already open, but close will return an error. Or return an error in both or silent in both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, will always error now
src/index.js
Outdated
} | ||
callback(null, config.Identity.PrivKey) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this method here? Repo should not assume a specific format of the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in here before, I just migrated the functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚮␡💥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Dragons inside ! 🐉 🐉 🐉
Integration with the new datastores, will have breaking api changes!
Will fix ipfs/js-ipfs#229 due to using https://github.com/dignifiedquire/lock-me for aquiring file system locks and exposing need methods to read and write the http api addr file.
ref ipfs/js-ipfs#787