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

Remove ready event from basic usage #1762

Closed
mikeal opened this issue Dec 7, 2018 · 7 comments · Fixed by #2094
Closed

Remove ready event from basic usage #1762

mikeal opened this issue Dec 7, 2018 · 7 comments · Fixed by #2094
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P3 Low: Not priority right now status/ready Ready to be worked

Comments

@mikeal
Copy link
Contributor

mikeal commented Dec 7, 2018

Now that we've started going down the "promises everywhere" rabbit hole it should be possible to remove waiting for the ready event from the default usage.

There's a few different ways we could go about this. Off the top of my head I think it would be nice to have a property on the object that is a promise that is resolved when ready.

let ipfs = new IPFS()
await ipfs.ready

And we could create a convenience method called create() that did this automatically.

let ipfs = await IPFS.create()

Thoughts?

@mikeal mikeal changed the title Remove ready event from default usage Remove ready event from basic usage Dec 7, 2018
@daviddias daviddias added the status/ready Ready to be worked label Dec 9, 2018
@alanshaw alanshaw added exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P3 Low: Not priority right now labels Dec 10, 2018
@haadcode
Copy link
Member

I'd personally love to see this happen! 👍I think adding/changing what is suggested here would improve the usability and readability a ton (=2 loc).

We had a discussion about it in the past in #1083 (see the different proposal for the API), but as you said @mikeal, things have changed now with using promises everywhere. Thanks for bringing it up again! ❤️

@pgte
Copy link
Contributor

pgte commented Dec 10, 2018

@mikeal As a frequent user of js-ipfs I also think something like this is really necessary — thanks for bringing this up!

I would like, though, to make a distinction between creating and starting, and would like to make .start() a method.
(The creation can only be called once, but starting would be idempotent.)

Because in some user-land packages we can be using IPFS node instances that we haven't created ourselves, we don't really know the start state, and simply doing something like this to ensure the node is started would help a lot on reducing the complexity of some processes:

// a) starts or,
// b) if starting, returns when started or
// c) if started, returns immediately
await ipfs.start() 

Also, I think we should decide between calling the state started versus ready. I'm fine with either, but await ipfs.start() is more intuituitive to me than await ipfs.ready().

Also, I think const ipfs = await IPFS.create(options) should, by default, start the node (unless options.start is false).

Nonetheless, the user could still ensure it's started by doing await start().

@mikeal
Copy link
Contributor Author

mikeal commented Dec 10, 2018

@pgte I ran into this just the other day calling await ipfs.start() when it was already started seeing the exception :)

I agree that it's confusing to have these two states, ready and started. My main question is, why is the ready state necessary? I understand that there are async things we want to do upon creation but why don't we just begin all of the API methods with await ipfs.ready and hide this state change from the user and effectively just queue up any method calls that are waiting for the ready state?

@pgte
Copy link
Contributor

pgte commented Dec 10, 2018

@mikeal that would be very helpful, (much alike a lot of node.js database clients where it queues operations or queries while not started).

One potential problem with this is that multiple operations or queries could fail if starting fails. But I guess users could always wait for started before starting any operation or query...

@mikeal
Copy link
Contributor Author

mikeal commented Dec 10, 2018

much alike a lot of node.js database clients where it queues operations or queries while not started

I don't think I would have had this idea had I not written this so many times in the level ecosystem :)

But I guess users could always wait for started before starting any operation or query...

If .start() has been called then we should wait for it to finish, if it hasn't been called we should throw if the operation can't complete without the node being started.

@mikeal
Copy link
Contributor Author

mikeal commented Dec 18, 2018

Looking through the code on this and we'll need to do this in stages.

  1. PR for a ready promise instantiated in the main constructor.
  2. Update API's to wait for ready promise
    2.1. Update depenedencies that implement top level APIs, like mfs.
    2.2. Update internal APIs.

For step 2, we'll want to have a discussion about whether or not we want to do this before the big async function refactor or after.

@oneEdoubleD
Copy link

I am happy to look at this over the holidays! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P3 Low: Not priority right now status/ready Ready to be worked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants