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

Describe when and why to use init: false #1263

Merged
merged 2 commits into from
Mar 24, 2018

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Mar 13, 2018

This adds some description to the "advanced options" documentation for the IPFS constructor detailing that init: false should really be used if a repo has already been initialized by IPFS and that simply calling repoInstance.init() doesn't init a repo with everything IPFS needs.

This partially handles #1245, but other parts of the experience (e.g. #1262) could also be improved.

Does this help clarify things? Or is it too confusing?

Is there anywhere else these options are documented that I need to update? (I couldn’t find anywhere, but I wouldn’t be surprised if I missed something.)

Add some description to the "advanced options" documentation for the `IPFS` constructor detailing that `init: false` should really be used if a repo has already been initialized *by IPFS* and that simply calling `repoInstance.init()` doesn't init a repo with everything IPFS needs.

This partially handles ipfs#1245.

License: MIT
Signed-off-by: Rob Brackett <work@robbrackett.com>
@Mr0grog Mr0grog force-pushed the fix/1245-explain-init-false-option branch from 18c43fe to 7419c7e Compare March 13, 2018 23:20
@dryajov
Copy link
Member

dryajov commented Mar 13, 2018

This looks good - thanks @Mr0grog!

@dryajov
Copy link
Member

dryajov commented Mar 13, 2018

Perhaps @diasdavid has more (historical) insight into that - but it might be worth adding that, even though IPFS takes a repo instance its not intended (IMHO) to be initialized outside of IPFS at all.

README.md Outdated
// // Be careful using this on an IPFSRepo instance you have
// // initialized yourself -- the IPFS constructor initializes
// // repos with extra options, so calling `init()` on an
// // IPFSRepo may not always be compatible with IPFS.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I don't understand what this comment is trying to prevent the user to do. Either a user knows what they are doing and they know supposed pitfall or they don't and the terms here don't give them any other help.

Copy link
Contributor Author

@Mr0grog Mr0grog Mar 17, 2018

Choose a reason for hiding this comment

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

OK. We clearly have users who don’t know what this is for. As noted in the summary, this is trying to prevent a user from having the kind of misunderstanding they had in #1245.

What language would you suggest better clarifies when to and not to use init: false? What should I say here that helps someone like that know they shouldn’t just:

const repo1 = new IPFSRepo('/tmp/ipfs-repo1');
repo1.init({}, (err) => {
  if (err) { console.log(err); return; }

  const node1 = new IPFS({
    repo: repo1,
    init: false,
    start: true
  });

  node1.on('start', () => {
    console.log('IPFS node 1 started.');
  });
})

Something like:

Use this if the repo has previously been initialized by passing it to new IPFS(). You should not attempt to call init() yourself on an IPFSRepo instance and then set init: false here.

?

Clearly this could go into a lot more detail, but because this is all inline in a code sample, there’s not a whole lot of room. There’s no other place to document this stuff, right?

Copy link

Choose a reason for hiding this comment

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

I think part of the confusion with repo is how you right code so that it works both the first time its called, and following times. init confused me a lot when I first saw it. Do I
a) set init=true - in which case I would assume it will clear the repo every time, losing all saved data or
b) set init=false in which case it will fail the first time.

I think most distributed IPFS based apps are going to want to default to "initialize a new repo if it doesn't exist, or if its incompatible (because IPFS has changed version and broken old repo's) and otherwise leave it alone". I still have no clue wither that is to set it to false, true or leave it missing (which is what we currently do. )

Some comments here about when to set it to true, when to false, and when to omit it would be useful.

@daviddias daviddias added the status/ready Ready to be worked label Mar 19, 2018
@ghost ghost assigned daviddias Mar 21, 2018
@ghost ghost added status/in-progress In progress and removed status/ready Ready to be worked labels Mar 21, 2018
@@ -194,7 +194,8 @@ const repo = <IPFS Repo instance or repo path>
const node = new IPFS({
repo: repo,
init: true, // default
// init: false,
// init: false, // You will need to set init: false after time you start instantiate a node as
// // the repo will be already initiated then.
Copy link
Member

Choose a reason for hiding this comment

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

@Mr0grog what about this? :)

Copy link
Member

@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.

I'll go ahead an merge this as it improves on existing docs. If a new review is needed, feel welcome to submit another PR :)

@daviddias daviddias merged commit 8e3ea44 into ipfs:master Mar 24, 2018
@ghost ghost removed the status/in-progress In progress label Mar 24, 2018
BazaarDog pushed a commit to BazaarDog/js-ipfs that referenced this pull request Mar 29, 2018
@Mr0grog Mr0grog deleted the fix/1245-explain-init-false-option branch April 3, 2018 15:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants