refactor: async iterables #2547
Conversation
|
Much excite, will digest and comment. I am 👎 on extending iterators though, convenient that the functions are, I don't think we should mess with core language features. |
|
N.B. this PR is NOT yet finished, I'm not sure it even runs and it has no tests, just looking for general feedback right now before I invest the last chunk of time to finish it.
Can you elaborate? Many iterable objects in JavaScript have other functions available on them, is it really "messing" with core language features to return an iterable object that has it's own functions? It's really beneficial to have a |
lidel
left a comment
lidel
left a comment
There was a problem hiding this comment.
Regarding reducing boilerplate related to first/last, I believe that key offenders are pretty specific:
- Adding a single thing with
ipfs.addand getting "array with one result" which needs to be unpacked. - Adding a directory tree with
ipfs.addjust to get the root CID (last item on the results array)
Personally I really like the uniformity of having first/last, but if its not the best way to do it, addressing those two cases I mentioned could get us mostly there.
| const iterator = pipe( | ||
| normaliseAddInput(source), | ||
| source => importer(source, ipld, opts), | ||
| transformFile(dag, opts), | ||
| preloadFile(preload, opts), | ||
| pinFile(pin, opts) | ||
| ) |
There was a problem hiding this comment.
I really like pipelines like this one.
Easy to understand what needs to happen just by scanning only a few lines without going too deep into details.
Doing more of this would make it easier to onboard new contributors to the codebase.
👏🏽 👍🏽 👏🏽 👍🏽
I believe the terms API and subsystem here is getting mixed. When a subsystem is not yet active, the API should not be exposed.
Nice
🤔 Overall, this PR proposes a lot of really good refactors to the internals, I like where it is going :) Thank you! |
Yes this should have read something more like "Restrict usage of subsystem APIs to appropriate lifecycle stage(s)"
I think it's better to throw a meaningful error explaining why the API cannot be accessed rather than the user getting
❤️ |
Always!
Sure!
The most obvious example that springs to mind is readable streams in node, but when this sort of thing happens I don't think the intention is for them to be consumed as streams and async iterators, rather streams or async iterators. So the interface a result presents should only do one thing and not leave the user in the position of:
..when in fact they haven't iterated asyncly over the result at all. Better to just let them pass the result to a function that extracts the first result or the last or the nth or whatever they need. Chances are they have a growing collection of modules for dealing with async iterators already anyway - piping, making things duplex, etc. |
add section on error codes Ref. ipfs/js-ipfs#2547 (comment)
* Update CONTRIBUTING_JS.md add section on error codes Ref. ipfs/js-ipfs#2547 (comment) * Update CONTRIBUTING_JS.md Co-Authored-By: Teri Chadbourne <[email protected]> * docs: add Error Codes example License: MIT Signed-off-by: Marcin Rataj <[email protected]> * Update CONTRIBUTING_JS.md Co-Authored-By: Teri Chadbourne <[email protected]>
|
@achingbrain @hugomrdias any chance you can review this PR? |
achingbrain
left a comment
achingbrain
left a comment
There was a problem hiding this comment.
I am still 👎 on extending iterators as it encourages people to not iterate and if they're not iterating, maybe iterators are the wrong thing to return.
That said I'd like to start playing with this API so gogogogogogo!
alanshaw
force-pushed
the
feat/ipfsx-boot-and-add
branch
from
0e27751 to
5f44bca
Compare
alanshaw
changed the title
This PR allows ipfsx to be used by calling `IPFS.create(options)` with `{ EXPERIMENTAL: { ipfsx: true } }` options.
It adds a single API method `add` that returns an iterator that yields objects of the form `{ cid, path, size }`. The iterator is decorated with a `first` and `last` function so users can conveniently `await` on the first or last item to be yielded as per the [proposal here](https://github.com/ipfs-shipyard/ipfsx/blob/master/API.md#add).
In order to boot up a new ipfsx node I refactored the boot procedure to enable the following:
1. **Remove the big stateful blob "`self`" - components are passed just the dependencies they need to operate.** Right now it is opaque as to which components require which parts of an IPFS node without inspecting the entirety of the component's code. This change makes it easier to look at a component and know what aspects of the IPFS stack it uses and consequently allows us to understand which APIs should be available at which points of the node's lifecycle. It makes the code easier to understand, more maintainable and easier to mock dependencies for unit tests.
1. **Restrict APIs to appropriate lifecycle stage(s).** This PR introduces an `ApiManager` that allows us to update the API that is exposed at any given point. It allows us to (for example) disallow `ipfs.add` before the node is initialized or access `libp2p` before the node is started. The lifecycle methods `init`, `start` and `stop` each define which API methods are available after they have run avoiding having to put boilerplate in every method to check if it can be called when the node is in a particular state. See #1438
1. **Safer and more flexible API usage.** The `ApiManager` allows us to temporarily change APIs to stop `init` from being called again while it is already running and has the facility to rollback to the previous API state if an operation fails. It also enables piggybacking so we don't attempt 2 or more concurrent start/stop calls at once. See #1061 #2257
1. **Enable config changes at runtime.** Having an API that can be updated during a node's lifecycle will enable this feature in the future.
**FEEDBACK REQUIRED**: The changes I've made here are a little...racy. They have a bunch of benefits, as I've outlined above but the `ApiManager` is implemented as a `Proxy`, allowing us to swap out the underlying API at will. How do y'all feel about that? Is there a better way or got a suggestion?
resolves #1438
resolves #1061
resolves #2257
refs #2509
refs #1670
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
|
Ok so I'm changing tact with this PR - people seem generally happy with the boot process approach and seeing as we now have a fully refactored This means we won't have a EXPERIMENTAL flag for ipfsx - as I think this can get sorted pretty quickly. PS. I've also removed the |
alanshaw
force-pushed
the
feat/ipfsx-boot-and-add
branch
from
b2dfbc6 to
cbdf9e6
Compare
BREAKING CHANGE: `ipfs.add` results now contain a `cid` property (a [CID instance](https://github.com/multiformats/js-cid)) instead of a string `hash` property. refs ipfs-inactive/interface-js-ipfs-core#394
|
All 👍🏽 for all the breaking changes, just unsure about:
Most users will fall flat on their faces by not having a convenient method. We've been there in the past, that is why we added these auxiliary methods in the first place. |
|
Note taken. Just one question - was it problematic because there wasn't a way to do it without rolling your own? Both
This will be on the README here, but it should definitely also be in the This keeps our API surface area small and will maybe encourage the community to create other Have I convinced you this is ok? P.S. thank you for taking the time to review ❤️ |
* refactor: convert pubsub API to async/await * refactor: switch wording to not enabled
alanshaw
added
awesome endeavour
exp/expert
* refactor: convert object API to async/await * fix: object export
* refactor: convert swarm API to async/await * refactor: use libp2p.connections
|
Oh #2547 (comment) nice! Looks super simple to me :) Thank you! |
|
superseded by #2726 |
This PR allows ipfsx to be used by callingIPFS.create(options)with{ EXPERIMENTAL: { ipfsx: true } }options.Edit: #2547 (comment)
It adds a single API method
addthat returns an iterator that yields objects of the form{ cid, path, size }.The iterator is decorated with a.firstandlastfunction so users can convenientlyawaiton the first or last item to be yielded as per the proposal hereIn order to boot up a new ipfsx node I refactored the boot procedure to enable the following:
self" - components are passed just the dependencies they need to operate. Right now it is opaque as to which components require which parts of an IPFS node without inspecting the entirety of the component's code. This change makes it easier to look at a component and know what aspects of the IPFS stack it uses and consequently allows us to understand which APIs should be available at which points of the node's lifecycle. It makes the code easier to understand, more maintainable and easier to mock dependencies for unit tests.ApiManagerthat allows us to update the API that is exposed at any given point. It allows us to (for example) disallowipfs.addbefore the node is initialized or accesslibp2pbefore the node is started. The lifecycle methodsinit,startandstopeach define which API methods are available after they have run avoiding having to put boilerplate in every method to check if it can be called when the node is in a particular state. See What does it mean to start an IPFS node? #1438ApiManagerallows us to temporarily change APIs to stopinitfrom being called again while it is already running and has the facility to rollback to the previous API state if an operation fails. It also enables piggybacking so we don't attempt 2 or more concurrent start/stop calls at once. See state machine, transitions and events #1061 Error: Not able to stop from state: stopping #2257FEEDBACK REQUIRED: The changes I've made here are a little...racy. They have a bunch of benefits, as I've outlined above but the
ApiManageris implemented as aProxy, allowing us to swap out the underlying API at will. How do y'all feel about that? Is there a better way or got a suggestion?resolves #1438
resolves #1061
resolves #2257
resolves #2509
resolves #1670
refs ipfs-inactive/interface-js-ipfs-core#394
BREAKING CHANGE:
IPFS.createNoderemovedBREAKING CHANGE: IPFS is not a class that can be instantiated - use
IPFS.create. An IPFS node instance is not an event emitter.BREAKING CHANGE: Instance
.readyproperty removed. Please useIPFS.createinstead.BREAKING CHANGE: Callbacks are no longer supported on any API methods. Please use a utility such as
callbackifyon API methods that return Promises to emulate previous behaviour.BREAKING CHANGE:
PeerIdandPeerInfoclasses are no longer statically exported fromipfs-http-clientsince they are no longer used internally.BREAKING CHANGE:
pin.addresults now contain acidproperty (a CID instance) instead of a stringhashproperty.BREAKING CHANGE:
pin.lsnow returns an async iterable.BREAKING CHANGE:
pin.lsresults now contain acidproperty (a CID instance) instead of a stringhashproperty.BREAKING CHANGE:
pin.rmresults now contain acidproperty (a CID instance) instead of a stringhashproperty.BREAKING CHANGE:
addnow returns an async iterable.BREAKING CHANGE:
addresults now contain acidproperty (a CID instance) instead of a stringhashproperty.BREAKING CHANGE:
addReadableStream,addPullStreamhave been removed.BREAKING CHANGE:
lsnow returns an async iterable.BREAKING CHANGE:
lsresults now contain acidproperty (whose value is a CID instance) instead of ahashproperty.BREAKING CHANGE:
files.readPullStreamandfiles.readReadableStreamhave been removed.BREAKING CHANGE:
files.readnow returns an async iterable.BREAKING CHANGE:
files.lsPullStreamandfiles.lsReadableStreamhave been removed.BREAKING CHANGE:
files.lsnow returns an async iterable.BREAKING CHANGE:
files.lsresults now contain acidproperty (whose value is a CID instance) instead of ahashproperty.BREAKING CHANGE:
files.lsno longer takes alongoption (in core) - you will receive all data by default.BREAKING CHANGE:
files.statresult now contains acidproperty (whose value is a CID instance) instead of ahashproperty.BREAKING CHANGE:
getnow returns an async iterable. Thecontentproperty value for objects yielded from the iterator is now an async iterable that yieldsBufferListobjects.BREAKING CHANGE:
stats.bwnow returns an async iterable.BREAKING CHANGE:
addFromStreamhas been removed. Useaddinstead.BREAKING CHANGE:
addFromFshas been removed. Please use the exportedglobSourceutility and pass the result toadd. See the glob source documentation for more details and an example.BREAKING CHANGE:
addFromURLhas been removed. Please use the exportedurlSourceutility and pass the result toadd. See the URL source documentation for more details and an example.BREAKING CHANGE:
name.resolvenow returns an async iterable. It yields increasingly more accurate resolved values as they are discovered until the best value is selected from the quorum of 16. The "best" resolved value is the last item yielded from the iterator. If you are interested only in this best value you could useit-lastto extract it like so:BREAKING CHANGE:
block.rmnow returns an async iterable.BREAKING CHANGE:
dht.findProvs,dht.provide,dht.putanddht.querynow all return an async iterable.BREAKING CHANGE:
dht.findPeer,dht.findProvs,dht.provide,dht.putanddht.querynow yield/return an object{ id: CID, addrs: Multiaddr[] }instead of aPeerInfoinstance(s).BREAKING CHANGE:
refsandrefs.localnow return an async iterable.BREAKING CHANGE:
object.datanow returns an async iterable that yieldsBufferobjects.BREAKING CHANGE:
pingnow returns an async iterable.BREAKING CHANGE:
repo.gcnow returns an async iterable.BREAKING CHANGE:
swarm.peersnow returns an array of objects with apeerproperty that is aCID, instead of aPeerIdinstance.BREAKING CHANGE:
swarm.addrsnow returns an array of objects{ id: CID, addrs: Multiaddr[] }instead ofPeerInfoinstances.BREAKING CHANGE:
block.statresult now contains acidproperty (whose value is a CID instance) instead of akeyproperty.BREAKING CHANGE:
bitswap.wantlistnow returns an array of CID instances.BREAKING CHANGE:
bitswap.statresult has changed -wantlistandpeersvalues are now an array of CID instances.BREAKING CHANGE: the
initoption passed to the IPFS constructor will now not take any initialization steps if it is set tofalse. Previously, the repo would be initialized if it already existed. This is no longer the case. If you wish to initialize a node but only if the repo exists, passinit: { allowNew: false }to the constructor.BREAKING CHANGE: removed
file lscommand from the CLI and HTTP API.BREAKING CHANGE: Delegated peer and content routing modules are no longer included as part of core (but are still available if starting a js-ipfs daemon from the command line). If you wish to use delegated routing and are creating your node programmatically in Node.js or the browser you must
npm install libp2p-delegated-content-routingand/ornpm install libp2p-delegated-peer-routingand provide configured instances of them inoptions.libp2p. See the module repos for further instructions: