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

[WIP] feat: ready promise #1878

Closed
wants to merge 2 commits into from
Closed

[WIP] feat: ready promise #1878

wants to merge 2 commits into from

Conversation

pkafei
Copy link
Contributor

@pkafei pkafei commented Feb 13, 2019

Related to #1762

@ghost ghost assigned pkafei Feb 13, 2019
@ghost ghost added the status/in-progress In progress label Feb 13, 2019
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.

Please add some tests to this feature to test its behavior and update the respective documentation. Thank you!

@mikeal
Copy link
Contributor

mikeal commented Feb 13, 2019

RE: documentation. Do we want to update the README to move all the examples to use this?

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Please could you address the linter error?:

/home/travis/build/ipfs/js-ipfs/src/core/index.js
  190:24  error  Arrow function should not return assignment  no-return-assign

@alanshaw
Copy link
Member

RE: documentation. Do we want to update the README to move all the examples to use this?

Yes, this should be the recommended way of obtaining a ready node.

Separately, I think we should stop advertising the callback API (both in the README and the examples) so predominantly. Instead just document that all the API methods support callbacks. I want new users to be using promises so that when #1670 lands it's easier for them to upgrade.

re: the examples here I think we should submit a separate PR to switch them all to using async await style, I'd rather not mix await ipfs.ready with callbacks.

@alanshaw alanshaw changed the title Feature: Ready Promise feat: ready promise Feb 18, 2019
@alanshaw
Copy link
Member

alanshaw commented Mar 5, 2019

@pkafei ping

@pkafei
Copy link
Contributor Author

pkafei commented Mar 5, 2019

@pkafei ping

@alanshaw Saw comment and will give detailed response shortly.

@pkafei
Copy link
Contributor Author

pkafei commented Mar 6, 2019

Yes, this should be the recommended way of obtaining a ready node.

Separately, I think we should stop advertising the callback API (both in the README and the examples) so predominantly. Instead just document that all the API methods support callbacks. I want new users to be using promises so that when #1670 lands it's easier for them to upgrade.

re: the examples here I think we should submit a separate PR to switch them all to using async await style, I'd rather not mix await ipfs.ready with callbacks.

Just want to reiterate the scope of this ticket. Here I'm implementing ready promise, and I'll update the README (docs) and add a test.

@mikeal and I plan on updating all the callbacks in the js-ipfs codebase to use async await in future tickets. I think it would be better for us to update all of the examples after we have refactored all of the callbacks to async await. Alternatively we can update the examples as we go.

@alanshaw alanshaw changed the title feat: ready promise [WIP] feat: ready promise Mar 13, 2019
@alanshaw
Copy link
Member

@pkafei do you need some help getting this finished?

@pkafei
Copy link
Contributor Author

pkafei commented Mar 20, 2019

@alanshaw Yes, I need help writing the test.

@alanshaw alanshaw mentioned this pull request May 23, 2019
3 tasks
@daviddias daviddias deleted the await-refactor branch July 17, 2019 17:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants