Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

async refactor - refactor internal methods as async #108

Merged
merged 3 commits into from
May 15, 2019

Conversation

kumavis
Copy link
Contributor

@kumavis kumavis commented Apr 29, 2019

This is an experiment in taking some of the excellent async refactors in #82 and applying them as non-breaking changes

The goal of this PR is to reduce the amount of unmerged code in the PR that is blocked by dependencies undergoing their own async refactors. Unmerged refactors will inevitably conflict with other ongoing contributions (e.g. #107)

A question - is the introduction of async/await if event used internally a breaking change to minimum platform support (e.g. node@6)? If so would something like @babel/register be considered a sufficient solution?

I tried to match code style and compared the feat/async-await and master branches with great care.

Some notes:

  • _sendCorrectionRecord was broken out as per feat/async-await
  • _getValueOrPeers return signature unchanged but _getValueOrPeersAsync returns an object as per feat/async-await
  • includes some doc improvements from fix: performance improvements #107
  • _findNProvidersAsync is not refactored into an async iterator as per feat/async-await but could be
  • additional portions of feat/async-await could be included without breaking changes, but I thought this should undergo review before continuing work

@ghost ghost assigned kumavis Apr 29, 2019
@ghost ghost added the status/in-progress In progress label Apr 29, 2019
@dirkmc
Copy link
Contributor

dirkmc commented Apr 30, 2019

@kumavis the async refactor will be a breaking change. We need to wait for the dependencies to be ready before we can merge it in. In the mean time there have been some other big changes in the way that the queries work (eg #107), so I think we will need to use that as a starting point rather than my refactors (some of which have already been ported into master).

I'm sorry we can't use this, as I see you've put a lot of effort into it.

@kumavis
Copy link
Contributor Author

kumavis commented May 3, 2019

I'm sorry we can't use this

@dirkmc can you clarify your reasoning here

In the mean time there have been some other big changes in the way that the queries work (eg #107), so I think we will need to use that as a starting point rather than my refactors (some of which have already been ported into master).

this is why I created this PR. in order to:

  • start using async/await internally asap
  • avoid long standing refactors that are blocked by dependencies
  • avoid long standing refactors that conflict with refactors

this PR was also made specifically with #107 in mind, with an eye on reducing conflicts

@kumavis kumavis force-pushed the feat/async-await-internally branch from d807ccb to 9786f05 Compare May 3, 2019 03:10
@kumavis kumavis force-pushed the feat/async-await-internally branch from 9786f05 to 87fa8c7 Compare May 3, 2019 03:13
@dirkmc
Copy link
Contributor

dirkmc commented May 3, 2019

@kumavis thanks for putting this PR together. We would prefer to refactor everything at once rather than piece-meal. There will likely be further changes to the DHT code before that big refactor can be made, so we would like to hold off on any more async/await refactoring for the time being. When we are ready to do so we will look at using this PR as part of the big refactor.

@kumavis
Copy link
Contributor Author

kumavis commented May 3, 2019

I strongly recommend against needing a big refactor in the first place if it can be avoided

We would prefer to refactor everything at once rather than piece-meal.

can you provide reasoning here. I think this is a bad idea and hinders contribution to this project

@kumavis
Copy link
Contributor Author

kumavis commented May 3, 2019

my philosophy:
👍 small incremental non-breaking improvements merged quickly, with small breaking changes of changing public interface
👎 large monolithic refactors requiring lots of coordination

@kumavis
Copy link
Contributor Author

kumavis commented May 6, 2019

@dirkmc can i get some more concrete criticism on this approach, thanks

@dirkmc
Copy link
Contributor

dirkmc commented May 6, 2019

@kumavis I'm just a contributor to this repo. There's an endeavour for converting a lot of our repos to async/await: ipfs/js-ipfs#1670. I'm sure they could use your help.

@kumavis
Copy link
Contributor Author

kumavis commented May 6, 2019

@dirkmc whoops, didnt realize. thanks

@vasco-santos
Copy link
Member

Thanks for this work @kumavis

Once I have some time, I will have a look and sync with you and @dirkmc as he has also been working a lot in this refactor for the DHT

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.

Code LGTM. @kumavis am I correct in assuming you're willing to send subsequent PRs to continue the conversion in bytesized (pun intended) chunks? You'll need to use #82 as inspiration (as you have bneen already) and include the additional fixes from there also.

package.json Outdated Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Looked into the changes and everything looks good!

Only needs the pify to be replaced, as stated by Alan!

@kumavis
Copy link
Contributor Author

kumavis commented May 15, 2019

@alanshaw @vasco-santos

  • replace pify with promisify-es6

This is ready to merge 🚀

Will continue breaking off little pieces of async refactor in separate PRs 😸

@kumavis kumavis requested a review from alanshaw May 15, 2019 08:26
@kumavis kumavis merged commit 042e852 into master May 15, 2019
@ghost ghost removed the status/in-progress In progress label May 15, 2019
@kumavis
Copy link
Contributor Author

kumavis commented May 15, 2019

@dirkmc thanks for kicking off this refactor!

@kumavis kumavis deleted the feat/async-await-internally branch May 15, 2019 15:18
@dirkmc
Copy link
Contributor

dirkmc commented May 15, 2019

Thanks for putting in the work to take this on @kumavis 💪👍

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