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

feat: ready promise #2094

Merged
merged 7 commits into from
Jul 17, 2019
Merged

feat: ready promise #2094

merged 7 commits into from
Jul 17, 2019

Conversation

alanshaw
Copy link
Member

This PR is rebased and extended from #1878. I've added tests for node.ready and IPFS.create, and updated the documentation.

REVIEWERS: Please be sure to review the README.md changes (github is not rendering the diff by default).

resolves #1762
closes #1878


This change is completely backwards compatible with existing methods of creating a ready IPFS node. It does two things:

  1. Adds functionalty for promised node creation and ready state
  2. Alters documentation to recommend usage of promise based APIs over callback APIs

Now is the right time to add this functionality. This will get existing users thinking about switching to promise based APIs and will onboard new users to using the Promise based APIs.

The idea is that this will lessen the impact of the async await/iterators refactor to users when it bubbles up to this level - users will have to change their applications to resolve API breakages but not also have to deal with a switch from callbacks to promises.

TODO (not necessarily as dependencies of this PR):

  • Update examples to use promise based APIs
  • Update interface-ipfs-core to use promise APIs in examples
  • PR to ipfs-http-client to add IPFS.create and node.ready so they can be used interchangably (?? do you think we really need this?)

@alanshaw
Copy link
Member Author

CI failing only on commitlint

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM

Also; hooray!

@dirkmc
Copy link
Contributor

dirkmc commented May 23, 2019

I second @achingbrain's hooray 🎉 Using an await is so much nicer than events.

Would it make sense for users to await ipfs.start() instead of await ipfs.ready? That way they won't accidentally get into the situation where they are awaiting for IPFS to be ready and expecting it to have started:

const ipfs = new IPFS({ start: false })
await ipfs.ready

If IPFS has already started, await ipfs.start() would just return immediately.

@alanshaw
Copy link
Member Author

For anyone watching this - I think @dirkmc's idea is good, but I just haven't got round to amending this PR yet.

@alanshaw
Copy link
Member Author

I've had a chance to look into await ipfs.start() vs await ipfs.ready.

I found that to support await ipfs.start() I'd have to do significantly more work in this PR to support the start command being able to piggyback on an already starting/initialising node (refs #2257) as well as work to automatically call init if it hasn't already been called.

I'd rather not expand the scope of this PR to include those works, since it has already hung around for way too long and I feel that...

a) most people will now use IPFS.create instead of await (new IPFS()).ready so if we want to remove ready in the future it won't be a huge problem, and is something that we can deprecate and support for a period anyway if needs be

and...

b) achieving the flow for await ipfs.start() will be a lot easier after the async/await refactor

So, I'm going to get this merged and released!

@alanshaw alanshaw mentioned this pull request Jul 17, 2019
54 tasks
alanshaw added 2 commits July 17, 2019 12:39
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

Ok it sounds like this is the best path forward at the moment 👍

@alanshaw alanshaw merged commit e0994f2 into master Jul 17, 2019
@alanshaw alanshaw deleted the await-refactor2 branch July 17, 2019 14:27
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.

Remove ready event from basic usage
4 participants