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

The one true API, the ipfsx upgrade path #2509

Closed
achingbrain opened this issue Oct 4, 2019 · 8 comments
Closed

The one true API, the ipfsx upgrade path #2509

achingbrain opened this issue Oct 4, 2019 · 8 comments

Comments

@achingbrain
Copy link
Member

Land #1670 🤯 🎊 😅
#2506 (comment)

But what does that mean though?

Something that bothers me is that we already support promises & callbacks in the API, so removing callbacks doesn't really give our users anything except a whole bunch of refactoring for those who do used the callbackified API. That can't be the end goal of #1670.

I think we should get on and implement ipfsx.

We've seen the API grow organically over time and it doesn't present the most streamlined interface. The key point is that IPFS is all about files, yet loads of the file operations are not top-level operations. Plus we have confusing caveat-filled weirdness with things like ipfs.ls and ipfs.files.ls.

The ipfsx approach is to finally unify the files API and move them all to the top level front and centre and have the low level stuff grouped by domain (e.g. block, dag, etc). It also removes readable streams, pull streams, etc giving us a smaller, more focused API surface area and (hopefully) smaller bundle size. I think we're all agreed that this is where we're heading.

As the internals follow the path of ipfs._addAsyncIterator we can start to implement ipfsx on top of them, then eventually split out the current API as a separate module to ease upgrading for people.

Step 1

Allow the user to expose the new API as an experimental flag. The old API will not be exposed - this may mean that some operations are impossible until they are implemented. Welcome to the bleeding edge.

const IPFS = require('ipfs')

const ipfs = await IPFS.create({
  EXPERIMENTAL: {
    ipfsx: true
  }
  // ... other options
})

// call the new API
await ipfs.mkdir('/somedir')

Step 2

Once the API is implemented, remove the old API entirely, deps and all. This should give us a leaner bundle and small surface area for those who have kept up to date.

Then create a wrapper for the new API that implements the old by calling new API methods and is callbackified/promisified/promise-nodified. This gives existing users a smooth upgrade path.

We should be able to verify it using today's interface-ipfs-core suite, though we should make tomorrow's version will only test the ipfsx interface.

const IPFS = require('ipfs')
const legacy = require('ipfs-legacy')

const ipfs = await legacy(IPFS).create({
  // ... other options
}))

// call the old API
await ipfs.files.mkdir('/somedir')
@achingbrain
Copy link
Member Author

achingbrain commented Oct 4, 2019

Or, more succinctly:

source

Am just finishing off async iterator functions for the remaining (non-mfs*) files api calls, then can start implementing the above if people agree with the approach.

* = these are async iterable already

@mikeal
Copy link
Contributor

mikeal commented Oct 7, 2019

The current implementation of ipfsx is pulling in the raw codecs from IPLD. You should probably just move to the new Block API instead https://github.com/ipld/js-block

You may also want to consider returning/yielding Block instances instead of CID’s and encoded data. In both cases, the data and CID are computed lazily in the new Block API and cached indefinitely. This will avoid double-hashing and double-encoding problems, and also avoids these activities entirely if they are never requested.

@mikeal
Copy link
Contributor

mikeal commented Oct 7, 2019

If you plan to continue to use the ipfsx code base I can send a PR that migrates to the Block API so you can see what it all looks like, I just won’t bother if this is actually going to be re-implemented when you integrate into js-ipfs.

@achingbrain
Copy link
Member Author

It would probably be reimplemented using the API in ipfs-shipyard/ipfsx as a template.

alanshaw added a commit that referenced this issue Oct 17, 2019
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 <alan.shaw@protocol.ai>
@daviddias
Copy link
Member

daviddias commented Oct 21, 2019

I'm not onboard with this. The hypothesis that giving yet another API as an option in addition to all the previous ones will benefit the users should not be assumed. I see it missing to recognize the amount of understanding necessary to even grok why these exist, why one is better than the other and why would a user need to care.

Creating a great API is like creating a great product, there needs to be a lot of "no" involved otherwise it is just a compromise soup. We've already agreed both in person (Berlin Dev Meetings and IPFS Camp + other hack weeks) and async (https://github.com/ipfs/interface-js-ipfs-core/issues/284) that the Files API needs to be refactored and that APIs such as Block & Object can either be deprecated or hidden.

If you want to explore a migrate path such as this (#2509), consider integrating in that migrate path something that clearly routes the users into a more happy place, rather than giving even more options and expecting them to differentiate well (specially when there are almost no docs).

My vote goes for having the new API in beta and call the old one legacy to make it evident that users should move away from it (or something that clearly points to the user what is new and good and what is old and should change).

@alanshaw
Copy link
Member

My vote goes for having the new API in beta and call the old one legacy to make it evident that users should move away from it (or something that clearly points to the user what is new and good and what is old and should change).

IMHO, releasing under the EXPERIMENTAL flag would give us the licence to release an incomplete core API, that may have to change before it becomes the blessed core API.

I take your point on adding another API though, and on reflection I think you're correct that we're not clearly moving users towards a more happy place in this way. At least, it would require a significant effort on our part to comminucate that EXPERIMENTAL.ipfsx will become the core API and it will be easier to use, more performant and a bunch of other reasons.

The problem I'm trying to solve is that adding ipfsx APIs is going to take time - we can't send a single PR with all the changes, it has to be done incrementally. I also don't think it's really an option for us to have a main release that actually removes functionality without an upgrade path (unless it's a planned removal of an API).

We could say 0.50.0 will have all the new core APIs, which would allow us to make releases as 0.50.0-beta.x as we land ipfsx APIs. That would give us space to use 0.40 - 0.49 for landing features and releasing with the old APIs - that's like 15 weeks - think we could do it.

We'd still use master when working on ipfsx but we'd have a temporary branch for the 0.50.0-beta.x releases that would change the default export to the new APIs not the old ones.

On a separate note I think "beta" is a loaded term and ideally I'd like the HTTP and CLI APIs to also be revamped before we move js-ipfs to beta.

Also, one final assertion - I'm not going to release anything related to these API changes that doesn't have great documentation and useful examples to accompany it 😁

@mikeal
Copy link
Contributor

mikeal commented Oct 21, 2019

My vote goes for having the new API in beta and call the old one legacy to make it evident that users should move away from it (or something that clearly points to the user what is new and good and what is old and should change).

It’s a little odd as a consumer to have a legacy API, a Beta API, and no “current” or “stable” API. I also think it’s just generally a good practice to put out new things early, gather feedback, iterate, and then once they stabilize consider replacing the existing API. No matter how good an idea is, and I think this new API is a great idea, you still want to have a discussion about replacing the current API based on the full implementation and feedback it has received rather than just replacing with a beta API that hasn’t been used in the wild much yet.

@achingbrain
Copy link
Member Author

The point of having ipfsx behind an EXPERIMENTAL flag is that it would be just that, experimental, so we can add, remove and change things and people would have to explicitly opt-in to that wild ride.

It’s a little odd as a consumer to have a legacy API, a Beta API, and no “current” or “stable” API

Agreed - the current API would continue to be the current API, until ipfsx is ready to become the current API (some time in February/early March) by @alanshaw's reckoning), at which point it would become the legacy API (and preferably would not be in the core codebase any more).

We can't really tell people to move off the current API until there's something for them to move to, and it has good answers for the majority of use cases.

we'd have a temporary branch for the 0.50.0-beta.x releases

Rather than it having different code, it might just have the EXPERIMENTAL.ipfsx flag on by default, then for 0.51.0 we could remove the legacy API code and release that as a separate wrapper module.

I'm not going to release anything related to these API changes that doesn't have great documentation and useful examples to accompany it

This should be standard practise for everything we do 🚀

alanshaw added a commit that referenced this issue Dec 9, 2019
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 <alan.shaw@protocol.ai>
alanshaw added a commit that referenced this issue Dec 10, 2019
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 <alan.shaw@protocol.ai>
alanshaw added a commit that referenced this issue Jan 23, 2020
A roundup of the following PRs:

* closes #2658
* closes #2660
* closes #2661
* closes #2668
* closes #2674
* closes #2676
* closes #2680
* test fixes and other fix ups

---

To allow us to pass the interface tests, the timeout option is now supported in the `object.get` and `refs` APIs. It doesn't actually cancel the operation all the way down the stack, but allows the method call to return when the timeout is reached.

https://github.com/ipfs/js-ipfs/pull/2683/files#diff-47300e7ecd8989b6246221de88fc9a3cR170

---

Supersedes #2724

---

resolves #1438
resolves #1061
resolves #2257
resolves #2509
resolves #1670
refs ipfs-inactive/interface-js-ipfs-core#394

BREAKING CHANGE: `IPFS.createNode` removed

BREAKING 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 `.ready` property removed. Please use `IPFS.create` instead.

BREAKING CHANGE: Callbacks are no longer supported on any API methods. Please use a utility such as [`callbackify`](https://www.npmjs.com/package/callbackify) on API methods that return Promises to emulate previous behaviour.

BREAKING CHANGE: `PeerId` and `PeerInfo` classes are no longer statically exported from `ipfs-http-client` since they are no longer used internally.

BREAKING CHANGE: `pin.add` results now contain a `cid` property (a [CID instance](https://github.com/multiformats/js-cid)) instead of a string `hash` property.

BREAKING CHANGE: `pin.ls` now returns an async iterable.

BREAKING CHANGE: `pin.ls` results now contain a `cid` property (a [CID instance](https://github.com/multiformats/js-cid)) instead of a string `hash` property.

BREAKING CHANGE: `pin.rm` results now contain a `cid` property (a [CID instance](https://github.com/multiformats/js-cid)) instead of a string `hash` property.

BREAKING CHANGE: `add` now returns an async iterable.

BREAKING CHANGE: `add` results now contain a `cid` property (a [CID instance](https://github.com/multiformats/js-cid)) instead of a string `hash` property.

BREAKING CHANGE: `addReadableStream`, `addPullStream` have been removed.

BREAKING CHANGE: `ls` now returns an async iterable.

BREAKING CHANGE: `ls` results now contain a `cid` property (whose value is a [CID instance](https://github.com/multiformats/js-cid)) instead of a `hash` property.

BREAKING CHANGE: `files.readPullStream` and `files.readReadableStream` have been removed.

BREAKING CHANGE: `files.read` now returns an async iterable.

BREAKING CHANGE: `files.lsPullStream` and `files.lsReadableStream` have been removed.

BREAKING CHANGE: `files.ls` now returns an async iterable.

BREAKING CHANGE: `files.ls` results now contain a `cid` property (whose value is a [CID instance](https://github.com/multiformats/js-cid)) instead of a `hash` property.

BREAKING CHANGE: `files.ls` no longer takes a `long` option (in core) - you will receive all data by default.

BREAKING CHANGE: `files.stat` result now contains a `cid` property (whose value is a [CID instance](https://github.com/multiformats/js-cid)) instead of a `hash` property.

BREAKING CHANGE: `get` now returns an async iterable. The `content` property value for objects yielded from the iterator is now an async iterable that yields [`BufferList`](https://github.com/rvagg/bl) objects.

BREAKING CHANGE: `stats.bw` now returns an async iterable.

BREAKING CHANGE: `addFromStream` has been removed. Use `add` instead.

BREAKING CHANGE: `addFromFs` has been removed. Please use the exported `globSource` utility and pass the result to `add`. See the [glob source documentation](https://github.com/ipfs/js-ipfs-http-client#glob-source) for more details and an example.

BREAKING CHANGE: `addFromURL` has been removed. Please use the exported `urlSource` utility and pass the result to `add`. See the [URL source documentation](https://github.com/ipfs/js-ipfs-http-client#url-source) for more details and an example.

BREAKING CHANGE: `name.resolve` now 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 use `it-last` to extract it like so:

```js
const last = require('it-last')
await last(ipfs.name.resolve('/ipns/QmHash'))
```

BREAKING CHANGE: `block.rm` now returns an async iterable.

BREAKING CHANGE: `block.rm` now yields objects of `{ cid: CID, error: Error }`.

BREAKING CHANGE: `dht.findProvs`, `dht.provide`, `dht.put` and `dht.query` now all return an async iterable.

BREAKING CHANGE: `dht.findPeer`, `dht.findProvs`, `dht.provide`, `dht.put` and `dht.query` now yield/return an object `{ id: CID, addrs: Multiaddr[] }` instead of a `PeerInfo` instance(s).

BREAKING CHANGE: `refs` and `refs.local` now return an async iterable.

BREAKING CHANGE: `object.data` now returns an async iterable that yields `Buffer` objects.

BREAKING CHANGE: `ping` now returns an async iterable.

BREAKING CHANGE: `repo.gc` now returns an async iterable.

BREAKING CHANGE: `swarm.peers` now returns an array of objects with a `peer` property that is a `CID`, instead of a `PeerId` instance.

BREAKING CHANGE: `swarm.addrs` now returns an array of objects `{ id: CID, addrs: Multiaddr[] }` instead of `PeerInfo` instances.

BREAKING CHANGE: `block.stat` result now contains a `cid` property (whose value is a [CID instance](https://github.com/multiformats/js-cid)) instead of a `key` property.

BREAKING CHANGE: `bitswap.wantlist` now returns an array of [CID](https://github.com/multiformats/js-cid) instances.

BREAKING CHANGE: `bitswap.stat` result has changed - `wantlist` and `peers` values are now an array of [CID](https://github.com/multiformats/js-cid) instances.

BREAKING CHANGE: the `init` option passed to the IPFS constructor will now not take any initialization steps if it is set to `false`. 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, pass `init: { allowNew: false }` to the constructor.

BREAKING CHANGE: removed `file ls` command 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-routing` and/or `npm install libp2p-delegated-peer-routing` and provide configured instances of them in [`options.libp2p`](https://github.com/ipfs/js-ipfs#optionslibp2p). See the module repos for further instructions:

- https://github.com/libp2p/js-libp2p-delegated-content-routing
- https://github.com/libp2p/js-libp2p-delegated-peer-routing
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants