-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: mfs implementation #1360
feat: mfs implementation #1360
Conversation
Minor note — would you please document the new |
a0fd5b9
to
259aa82
Compare
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.
You're right, we should really be reading config from an existing config file and overriding with --enable-XXX-experiment
or env vars when set.
I'm ok with loading MFS functions up like this from a separate module. Was your plan to leave it like this or eventually merge the ipfs-mfs
repo with this one when it's all done?
src/cli/bin.js
Outdated
|
||
return commandObject | ||
} | ||
}) |
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.
Would you mind explaining what's going on here?
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 preventing any of the existing ipfs files <command>
commands from loading if enableMfs
is truthy.
|
||
describe('file ls', () => runOnAndOff((thing) => { | ||
let ipfs | ||
|
||
before(function () { | ||
this.timeout(50 * 1000) | ||
ipfs = thing.ipfs | ||
return ipfs('files add -r test/fixtures/test-data/recursive-get-dir') | ||
return ipfs('add -r test/fixtures/test-data/recursive-get-dir') |
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.
So is this suggesting that add
, get
and cat
would no longer be available under the files
namespace?
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.
Yes, this is consistent with go-ipfs
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.
Let's think more about this...
I would be totally down to move forward with ipfs/specs#98 in JS land (both js-ipfs and js-ipfs-api). In fact, I believe we are reaching a time where we need to start pushing forward better and polished APIs, given that we live in a different ecosystem and we need to make things that work for our user needs.
An advantage to ourselves is that we have a SPEC and tests, making this transition smooth and more pleasant.
@diasdavid's preference was for a separate module so there are no plans to merge it into this one AFAIK. |
@achingbrain can we drop the experimental flag? This should work with go-ipfs right off the bat. |
Sure, that’d certainly make things a bit more straightforward.
… On 1 Jun 2018, at 13:04, David Dias ***@***.***> wrote:
@achingbrain can we drop the experimental flag? This should work with go-ipfs right off the bat.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
We are going all-in with mfs and not having it behind a flag: ipfs/js-ipfs#1360 (comment)
We are going all-in with mfs and not having it behind a flag: ipfs/js-ipfs#1360 (comment)
2e73d97
to
4fed9c0
Compare
8c6a548
to
4406bb7
Compare
aec7c8a
to
a1d2a01
Compare
May be too specific, but is there an interop test that checks if |
94228dd
to
bd4d34b
Compare
@diasdavid @lidel the interop tests are here: ipfs/interop#25 and there are a whole bunch that assert on CIDs generated by go & 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 is incredible, thanks for your patients @achingbrain
} else if (arg.error && arg.error.message) { | ||
print(arg.error.message) | ||
} else { | ||
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output') |
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 would be great to have some consistency here - the rest of the cli debug prefixes are cli:*
and elsewhere in the code we have jsipfs:*
. One for a separate PR 😉
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.
I'd love it if we could standardise on one prefix for all ipfs
related DEBUG
values. Just doing ipfs:*
instead of ipfs:*,ipfsd-ctl:*,ipfs-api:*,etc
is my dream.
src/cli/bin.js
Outdated
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output') | ||
} | ||
|
||
process.exit(1) |
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.
We always called cleanup
before even if there was an error. I think this needs to be added here too.
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.
We should probably register a process exit/error handler to do the cleanup instead, but we'd need a synchronous way of closing the repo which I don't think exists yet.
src/cli/commands/dag/get.js
Outdated
@@ -41,7 +41,7 @@ module.exports = { | |||
// * reads as 'agree in' | |||
if (node._json) { | |||
delete node._json.multihash | |||
node._json.data = '0x' + node._json.data.toString('hex') | |||
node._json.data = node._json.data.toString('base64') |
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.
Is this change meant to be in this PR? Why not also for the if test below?
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.
Probably could be separate. go prints out the data as base64 encoded, we print it out as hex.
src/core/index.js
Outdated
const mfs = components.mfs(this) | ||
|
||
Object.keys(mfs).forEach(key => { | ||
if (mfs.hasOwnProperty(key)) { |
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.
Minor: I believe Object.keys
iterates over own properties so I think this check is redundant, unless I'm missing something?
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.
From my understanding Object.keys
returns all properties, including inherited ones whereas hasOwnProperty
will only return true if it's a non-inherited property. Happy to change this if you prefer it the other way.
test/browser.js
Outdated
if (isWebWorker) { | ||
// https://github.com/Joris-van-der-Wel/karma-mocha-webworker/issues/4 | ||
global.MFS_DISABLE_CONCURRENCY = true | ||
} |
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.
Since there is a requirement to either set this variable or do some setup on the main thread (for webworkers) we should mention it and link to it from the js-ipfs README so that it's easily discoverable and we don't get issues opened about it here.
Also a separate PR to add a examples for using MFS in webworkers with/without concurrency is needed and would be very 🚀 🎸 ✨ 💖
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.
I've added a paragraph to the README, will create an example in a separate PR.
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.
What does MFS with concurrency mean?
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.
nvm. Read the paragraph.
test/core/interface.spec.js
Outdated
|
||
tests.key(CommonFactory.create({ | ||
spawnOptions: { | ||
args: ['--pass ipfs-is-awesome-software'], | ||
initOptions: { bits: 512 } | ||
initOptions: { bits: 1024 } |
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 is for faster boot - defaultCommonFactory
uses 512 also, is this change needed?
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.
I ran the tests against the go daemon and it refused to start up with 512 and said it should be 1024 as a minimum. Can change it back if you prefer.
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.
Yep in js-ipfs-api it's set to 1024 it tests against go-ipfs, but these tests only run against js-ipfs and there's no restriction in place currently.
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 probably should be, for the sake of interop though, right?
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.
Yes good point, lets not slow down the tests unnecessarily until that PR comes in though. I'll create an issue for adding it so we can track it.
README.md
Outdated
@@ -135,6 +136,20 @@ You can also load it using a `<script>` using the [unpkg](https://unpkg.com) CDN | |||
``` | |||
Inserting one of the above lines will make an `Ipfs` object available in the global namespace. | |||
|
|||
### Use with Web Workers | |||
|
|||
The `ipfs.files.*` [MFS](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#mutable-file-system) commands use LevelDB to store the CID that corresponds to the current root of the file system. LevelDB has no concurrency control so a read/write lock at the application level is necessary. With single-process and [Node.js cluster](https://nodejs.org/api/cluster.html) based apps this is completely transparent but [Web Workers](https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API) require a little extra setup. Use the [`observable-webworkers`](https://www.npmjs.com/package/observable-webworkers) module to allow communication with any Web Workers your application creates: |
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.
I see. This is so that if multiple WebWorkers are spawn, the store is shared?
Note that this is valid for anything that IPFS stores really, in the browser it is IndexedDB for 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.
Yes, if multiple threads mutate the MFS graph at the same time they'll result in different hashes for the root of the graph creating a last-update-wins race condition. There needs to be some sort of gatekeeper to resolve this.
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.
Multiple threads, each with its IPFS Node or just one IPFS node?
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.
I've flipped this round so that you only need to do the config dance if you've spawned a node on the main process then forked (e.g. node clusters) or spawned nodes on the UI thread and web workers (e.g. browsers) all using the same repo - which hopefully shouldn't be very common.
I'm not sure we should encourage this in the browser at least so I've left it undocumented for the time being but we can discuss this during the locking session at the dev meetup next week.
2f11def
to
3933a3a
Compare
e9bd96e
to
95f99eb
Compare
License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
Not ready for merging yetbut I'd love some feedback on the approach. I've placed the mfs commands behind a flag named--enable-mfs-experiment
- if you enable this it will cause theipfs files
subcommands to be the same as the go implementation.