Skip to content
This repository was archived by the owner on Oct 23, 2022. It is now read-only.

Simplified node creation #343

Closed
wants to merge 5 commits into from
Closed

Conversation

ljedrz
Copy link
Member

@ljedrz ljedrz commented Aug 27, 2020

The UninitializedIpfs struct neither has to be created using async nor is it necessary as a proxy object at all. We can simplify things even further, by not having the proxy IpfsInner object either.

This is a draft to "gauge the room" - if this is indeed a preferable course of action, I'll fix the docs, tests etc.

ljedrz added 4 commits August 27, 2020 11:05
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
@ljedrz ljedrz requested a review from koivunej August 27, 2020 11:09
}

/// Initialize the ipfs node. The returned `Ipfs` value is cloneable, send and sync, and the
/// future should be spawned on a executor as soon as possible.
pub async fn start(self) -> Result<(Ipfs<Types>, impl Future<Output = ()>), Error> {
pub async fn start(&mut self) -> Result<impl Future<Output = ()>, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The story behind of UninitializedIpfs was probably to originally work around some Send or Sync bounds related to some type which is no longer an issue however I think the real benefit of UninitalizedIpfs was that there was no start on Ipfs which would be called multiple times as the move semantics of the lang give rise to nicer APIs.

What does one do with an Ipfs value if the start fails?

Could the start be moved to IpfsOptions...? As a build method or something? Or maybe in place of new?

Copy link
Member Author

Choose a reason for hiding this comment

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

What does one do with an Ipfs value if the start fails?

same applies to UninitializedIpfs, no? I'm all for improving the naming here, I'll think about it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well UninitializedIpfs is a worthless value in itself except for starting, and start consumed itself, so you don't have a value left.

Copy link
Member Author

@ljedrz ljedrz Aug 27, 2020

Choose a reason for hiding this comment

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

while working on ipfs-substrate I had a case where it was easy to spawn tasks, but you had no control over what they were returning, making the setup practical for long-running tasks, but not for instantiating stuff like here; with this change the benefit is that Ipfs can be created (and kept) in a non-async context, while the background task can be consumed/concealed away; actually, since Ipfs is Clone, this can still consume self (and additionally return Ipfs), would that be better?

Signed-off-by: ljedrz <ljedrz@gmail.com>
@ljedrz
Copy link
Member Author

ljedrz commented Aug 28, 2020

Obviated by #345.

@ljedrz ljedrz closed this Aug 28, 2020
@ljedrz ljedrz deleted the less_async_creation branch August 30, 2020 11:22
koivunej pushed a commit to eqlabs/rust-ipfs that referenced this pull request Sep 23, 2020
... instead of Ipfs. this was the original idea in rs-ipfs#343 perhaps.
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.

2 participants