-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
==========================================
- Coverage 93.51% 85.97% -7.54%
==========================================
Files 1 2 +1
Lines 185 164 -21
==========================================
- Hits 173 141 -32
- Misses 12 23 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major here.
src/index.js
Outdated
} | ||
|
||
_put (cid, node, callback) { | ||
callback = callback || noop | ||
|
||
waterfall([ | ||
(cb) => this._getFormat(cid.codec, cb), | ||
async () => { | ||
return this._getFormat(cid.codec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work? I would have though you'd need to do something like:
waterfall([
async (cb) => {
try {
cb(null, await this._getFormat(cid.codec))
} catch (err) {
cb(err)
}
},
...
])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async
module supports async/await style coding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this only works with non-transpiled async functions? "Using ES2017 async functions" -> https://caolan.github.io/async/ (can't deep link, sorry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Does this mean that tests pass as we run webpack with different configurations for development and production? I even think we don't really run tests with the Browser build we publish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Moz async functions are supported everywhere except IE, and our contributing guidelines say which Node version we support but not which browser types/versions.
Aegir uses babel/preset-env
which I guess means it'll use native async unless you use IE, which will get a polyfill and this will break.
@alanshaw @olizilla @lidel - what browsers we officially support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the browser build we currently use this browser list:
>1%, last 2 versions, Firefox ESR, not ie < 11
Which contains mobile browsers that don't support async/await.
Is there a blessed async/bluebird style library we're going to use for async/iterable manipulations? Most of the methods in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I've addressed everything.
loadFormat (codec, callback) { | ||
if (codec !== 'dag-cbor') return callback(new Error('unexpected codec')) | ||
setTimeout(() => callback(null, dagCBOR)) | ||
async loadFormat (codec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally look quite often into the tests of projects to get examples of the API. Hence I wanted to make it explicit that generally an async function is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that there's no mention of "fancy iterator" in the return types of the API doc.
Also please can we think of a better name than "fancy iterator"?
src/index.js
Outdated
this.support.rm = (multicodec) => { | ||
if (this.resolvers[multicodec]) { | ||
delete this.resolvers[multicodec] | ||
if (options.loadFormat === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably better to typeof options.loadFormat !== 'function'
here
options = {} | ||
this.resolvers[codec] = { | ||
resolver: format.resolver, | ||
util: format.util | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not allow add multiple then return this
for chaining?
const codec = multicodec.getCode(codecBuffer) | ||
if (this.resolvers[codec]) { | ||
const codecName = multicodec.print[codec] | ||
throw new Error(`Resolver already exists for codec "${codecName}"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just observing that we're being strict on adding a new format but not for remove - I cannot add if already exists, but it's not a problem if I try to remove a format that doesn't exist...
Personally I'd probably remove this check, coding around this with try+addFormat+catch+removeFormat+addFormat is painful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how often you really want to replace an existing format. I see it as a gatekeeper to prevent hard to debug bugs, where accidentally some format is replaced.
src/index.js
Outdated
// get block | ||
// use local resolver | ||
// update path value | ||
this.bs.get(cid, (err, block) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use promisify
so we don't have to mix callbacks and promises and then you don't have to return new Promise
, you don't have to try/catch around awaits in order to call resolve
and this function gets way shorter, easier to read and understand. e.g.
resolve (cid, path) {
if (!CID.isCID(cid)) {
throw new Error('`cid` argument must be a CID')
}
if (typeof path !== 'string') {
throw new Error('`path` argument must be a string')
}
const next = async () => {
// End iteration if there isn't a CID to follow anymore
if (cid === null) {
return { done: true }
}
const format = await this._getFormat(cid.codec)
// get block
// use local resolver
// update path value
const block = await promisify(this.bs.get)(cid)
const result = await promisify(format.resolver.resolve)(block.data, path)
// Prepare for the next iteration if there is a `remainderPath`
path = result.remainderPath
let value = result.value
// NOTE vmx 2018-11-29: Not all IPLD Formats return links as
// CIDs yet. Hence try to convert old style links to CIDs
if (Object.keys(value).length === 1 && '/' in value) {
value = new CID(value['/'])
}
cid = CID.isCID(value) ? value : null
return {
done: false,
value: {
remainderPath: path,
value
}
}
}
return fancyIterator(next)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Promisifying" things that way doesn't work. Here's a minimal example:
'use strict'
const promisify = require('util').promisify
const CID = require('cids')
const IpfsBlockService = require('ipfs-block-service')
const IpfsRepo = require('ipfs-repo')
const ipfsRepoPath = '/tmp/getpromisifiedblocks'
const repo = new IpfsRepo(ipfsRepoPath, { init: true })
repo.init({}, (error) => {
if (error) {
throw error
}
repo.open(async (error) => {
if (error) {
throw error
}
const blockService = new IpfsBlockService(repo)
const cid = new CID('zdpuAvwqFGiEVdAQjymUUyJzJK5nuZwn2QKw3WSj1vDdoZmRX')
const block = await promisify(blockService.get)(cid)
// Works (well it throws the error we would expect
//blockService.get(cid, (error, block) => {
// if (error) {
// throw error
// }
//})
})
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I would've had @alanshaw's sk1llz I would've known to just use const block = await promisify(blockService.get.bind(blockService))(cid)
src/index.js
Outdated
return callback(err) | ||
|
||
let blocks | ||
const next = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, just make this an async
function and use promisify. Lets not mix callbacks and promisies.
src/index.js
Outdated
|
||
const next = () => { | ||
// End iteration if there are no more nodes to put | ||
if (nodes.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if we allow iterables we can't assume they have a length property - you need to iterate over it to determine the length, and ideally combine the iteration over the input with the operation to create a DAG node. I was anticipating this to be implemented something like:
function put (nodes) {
// Generator function that creates a generator - a special iterator
const gen = async function * () {
// We iterate over `nodes`
// `for await` works with iterators AND async iterators so `nodes` can be
// created asynchronously and canceled half way through if we like!
for await (const node of nodes) {
const cid = await serializeAndPut(node)
yield cid // halt execution of this loop, returning the `cid` of this new node
}
}
return gen()
}
src/index.js
Outdated
callback(null, format) | ||
}) | ||
const format = await this.loadFormat(codec) | ||
this.resolvers[codec] = format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addFormat
we pick out resolver
and util
- can be consistent (or use addFormat
?)
src/index.js
Outdated
}) | ||
const format = await this.loadFormat(codec) | ||
this.resolvers[codec] = format | ||
return format | ||
} | ||
|
||
_put (cid, node, callback) { | ||
callback = callback || noop | ||
|
||
waterfall([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
promisify
please!
src/index.js
Outdated
* @return {Object} = Returns the deserialized node | ||
*/ | ||
async _deserialize (block) { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
promisify
!
iterator.last = () => exports.last(iterator) | ||
iterator.all = () => exports.all(iterator) | ||
return iterator | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this is a decorator in IPFSX is because generators have return
and throw
functions that are called automatically by a for await
loop to allow the generator to clean up when execution is aborted/finished.
const iterator = {
return () {
console.log('return called')
return { done: true }
},
next () {
return { done: false, value: 1 }
},
[Symbol.iterator] () {
return iterator
}
}
async function main () {
let i = 0
for await (const value of iterator) {
i++
if (i === 3) break
console.log(value)
}
}
main()
// output:
// 1
// 1
// return called
If we create a custom async iterator here that isn't a generator then breaking out of a loop that uses it will not call return, all that will happen is that next
will not be called again. I don't think this is a problem right now with the code in this PR...but there could be issues here e.g. fs.createReadStream
with a transform that converts raw data into an IPLD node, passed to ipld.put
, halting half way would never end the stream, could cause the whole of the file content to be buffered into memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With "being a decorator" you mean that you're extending/modifying the iterator you passed in? I previously had that until @achingbrain mentioned that it's bad style to modify things you pass in (I agree with that). But in this case it might make sense.
What if fancyiterator just implements a return and throw? I know it won't be general purpose but good enough for js-ipld, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically a decorator would wrap the instance being decorated instead of modifying it, but you could argue it's a side-effect of a type system that doesn't allow modification at runtime.
@alanshaw I think I addressed all your comments (btw: they were great, the code is much better now). I also added some information about the extended iterators that are returned by the API (see https://github.com/ipld/js-ipld/blob/f5fa4ceba7dc1f85ba771dce00b1679fc099d990/README.md#api). |
"aegir": "^17.1.1", | ||
"bitcoinjs-lib": "^4.0.2", | ||
"aegir": "^18.0.3", | ||
"async": "^2.6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for the tests. The tests should be refactored as there's a lot of duplicated code, but I'd do that at a later stage.
src/index.js
Outdated
@@ -1,21 +1,16 @@ | |||
'use strict' | |||
|
|||
const promisify = require('util').promisify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
js-ipfs
uses promisify-es6
- it would be nice to not put another promisify implementation in the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could Node.js native things be polyfilled with something like promisify-es6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it seems to be compatible, so it was easy enough to switch.
src/index.js
Outdated
// NOTE vmx 2018-11-29: Not all IPLD Formats return links as | ||
// CIDs yet. Hence try to convert old style links to CIDs | ||
if (Object.keys(value).length === 1 && '/' in value) { | ||
value = new CID(value['/']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor point but this will throw if value['/']
is not a CID. Should we instead try/catch
incase this is actually data that just happens to be an object with one property that is /
?
yield { | ||
remainderPath: path, | ||
value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 this is HAWT 😆
if (!Array.isArray(cids)) { | ||
return callback(new Error('Argument must be an array of CIDs')) | ||
get (cids) { | ||
if (!typical.isIterable(cids) || typical.isString(cids) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typical.isIterable
does not check for async iterable i.e. [Symbol.asyncIterator]
https://github.com/75lb/typical/blob/cee19519fadf276ba190e9543ead7695ebb119dc/index.js#L192-L198
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, good catch!
const block = await promisify(this.bs.get.bind(this.bs))(cid) | ||
const format = await this._getFormat(block.cid.codec) | ||
const node = await promisify(format.util.deserialize)(block.data) | ||
yield node | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually we want to pass our cids
to the blockstore:
for await (const block of this.bs.get(cids)) { /* ... */ }
This is so the blockstore can look at the iterator and (for example) use leveldb "batch" or optimise the number of calls to the DHT.
From there my concern is that we're deserializing blocks in series whereas before we were doing all this in parallel and that'll be a huge perf hit.
I don't know how to do this with async iterators but there must be a simple way...I'll get back to you...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance is not a concern for IPLD atm. It's not optimized at all yet.
let formatImpl | ||
|
||
const generator = async function * () { | ||
for await (const node of nodes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is ok for now (new functionality) but in the future we need consume these in parallel like https://github.com/pull-stream/pull-paramap
return e | ||
// Only follow links if recursion is intended | ||
if (options.recursive) { | ||
cid = await maybeRecurse(block, treePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is super complicated and difficult to follow. Why don't we just call something like this here?:
for await (const path of this.tree(cid, treePath, userOptions)) {
yield path
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tree()
function will change a lot once the IPLD Formats use a new API. For now for me it's only important that it works.
const serialized = await promisify(format.util.serialize)(node) | ||
const block = new Block(serialized, cid) | ||
await promisify(this.bs.put.bind(this.bs))(block) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok for now but the block store needs to take a iterator and return an iterator that yields the blocks you put in:
put (nodes) {
return this.bs.put(serialize(nodes))
}
@alanshaw thanks for the thorough review. I've pushed new commits, all comments should be either addressed by those commits or my comments. |
My PR on typical to add async iterator support was merged an released! |
BREAKING CHANGE: `resolve()` replaces parts of `get()`. The API docs for it: > Retrieves IPLD Nodes along the `path` that is rooted at `cid`. - `cid` (`CID`, required): the CID the resolving starts. - `path` (`IPLD Path`, required): the path that should be resolved. Returns an async iterator of all the IPLD Nodes that were traversed during the path resolving. Every element is an object with these fields: - `remainderPath` (`string`): the part of the path that wasn’t resolved yet. - `value` (`*`): the value where the resolved path points to. If further traversing is possible, then the value is a CID object linking to another IPLD Node. If it was possible to fully resolve the path, `value` is the value the `path` points to. So if you need the CID of the IPLD Node you’re currently at, just take the `value` of the previously returned IPLD Node.
BREAKING CHANGE: your custom format loading function needs to be an async now. So the signature for `options.loadFormat` is no longer: function (codec, callback) but async functiont (codec)
Instead of storing a codec by their name, use their code instead. The tests are changed from `base1` to `blake2b-8` as codecs for different bases are no longer part of multicodec, but are part of multibase now. BREAKING CHANGE: The `codec` parameter in `options.loadFormat()` is a number Instead of returnign the name of the codec as string, the codec code (a number) is now returned. So if you e.g. check within the function for a certain format, it changes from: async loadFormat (codec) { if (codec !== 'dag-cbor') … } To: async loadFormat (codec) { if (codec !== multicodec.DAG_CBOR) … }
BREAKING CHANGE: The API of `put()` changes. The API docs for it: > Stores the given IPLD Nodes of a recognized IPLD Format. - `nodes` (`Iterable<Object>`): deserialized IPLD nodes that should be inserted. - `format` (`multicodec`, required): the multicodec of the format that IPLD Node should be encoded in. - `options` is applied to any of the `nodes` and is an object with the following properties: - `hashAlg` (`multicodec`, default: hash algorithm of the given multicodec): the hashing algorithm that is used to calculate the CID. - `cidVersion` (`boolean`, default: 1): the CID version to use. - `onlyHash` (`boolean`, default: false): if true the serialized form of the IPLD Node will not be passed to the underlying block store. Returns an async iterator with the CIDs of the serialized IPLD Nodes.
BREAKING CHANGE: `get()` is replacing the `getMany()` function. The API docs for it: > Retrieve several IPLD Nodes at once. - `cids` (`Iterable<CID>`): the CIDs of the IPLD Nodes that should be retrieved. Returns an async iterator with the IPLD Nodes that correspond to the given `cids`. Throws an error if a IPLD Node can’t be retrieved.
BREAKING CHANGE: `remove()` has a new API. The API docs for it: > Remove IPLD Nodes by the given `cids` - `cids` (`Iterable<CID>`): the CIDs of the IPLD Nodes that should be removed. Throws an error if any of the Blocks can’t be removed. This operation is *not* atomic, some Blocks might have already been removed.
BREAKING CHANGE: They replace the `support.add()` and `support.rm()` functions. The API docs for it: `.addFormat(ipldFormatImplementation)`: > Add support for an IPLD Format - `ipldFormatImplementation` (`IPLD Format`, required): the implementation of an IPLD Format. `.removeFormat(codec)`: > Remove support for an IPLD Format - `codec` (`multicodec`, required): the codec of the IPLD Format to remove.
BREAKING CHANGE: This replaces the `treeStream()` function. The API docs for it: > Returns all the paths that can be resolved into. - `cid` (`CID`, required): the CID to get the paths from. - `path` (`IPLD Path`, default: ''): the path to start to retrieve the other paths from. - `options`: - `recursive` (`bool`, default: false): whether to get the paths recursively or not. `false` resolves only the paths of the given CID. Returns an async iterator of all the paths (as Strings) you could resolve into.
This way you can chain addFormat()/removeFormat() calls.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
js-ipfs is using the promisify-es6 module, so it makes sense that we use it as well.
We are still supporting old style links, i.e. JSON where `/` as key signals a link. Though the JSON could could also contain such a key without having a CID as value. Instead of throwing an error, treat it as not being a link.
More recent versions of typical detect async itertators as an iterable.
// get block | ||
// use local resolver | ||
// update path value | ||
const block = await promisify(this.bs.get.bind(this.bs))(cid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do the promisification of the blockstore methods in the constructor instead of repeatedly during loops (e.g. here, during .get
, .remove
, .tree
, etc)?
e.g.
class IPLDResolver {
constructor (opts) {
this.bs = {
get: promisify(opts.bs.get.bind(opts.bs)),
put: promisify(opts.bs.put.bind(opts.bs)),
delete: promisify(opts.bs.delete.bind(opts.bs)),
// ...etc
}
// ...etc
}
}
Alternatively use something like Bluebird.promisifyAll until the lower level libraries are promise-aware?
|
||
resolver.bs._repo.blocks.has(cid2, cb) | ||
} | ||
(node, cb) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking awkward within a waterfall
, maybe it should be converted to an async test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were just adapted with minimal changes. The tests are in a bad shape currently and need a bigger refactor (there's lots of duplicated code). But I'd like to do it after this change is merged.
expect(sameAsNode.data).to.deep.equal(node.data) | ||
return remove() | ||
|
||
async function remove () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these wrapped up in their own function? the two lines should probably just be at the top level of this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests were just adapted with minimal changes. The tests are in a bad shape currently and need a bigger refactor (there's lots of duplicated code). But I'd like to do it after this change is merged.
src/index.js
Outdated
* @param {number} format - The multicodec of the format that IPLD Node should be encoded in. | ||
* @param {Object} [userOptions] - Options is an object with the following properties. | ||
* @param {number} [userOtions.hashAlg=hash algorithm of the given multicodec] - The hashing algorithm that is used to calculate the CID. | ||
* @param {number} [userOptions.cidVersion=1]`- The CID version to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erroneous '`'
src/index.js
Outdated
* @param {number} format - The multicodec of the format that IPLD Node should be encoded in. | ||
* @param {Object} [userOptions] - Options are applied to any of the `nodes` and is an object with the following properties. | ||
* @param {number} [userOtions.hashAlg=hash algorithm of the given multicodec] - The hashing algorithm that is used to calculate the CID. | ||
* @param {number} [userOptions.cidVersion=1]`- The CID version to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erroneous '`'
BREAKING CHANGE: put/get/remove functions are renamed This commit introduces single item functions which are called `put()`/`get()`,`remove()`. In order to put, get or remove multiple items you need to call `putMany()`,`getMany()`/`removeMany()` now.
This is a huge PR. I've tried to keep the commits self-contained. So I suggest that your review one commit after another instead of the full PR. I'm happy to change the individual commits as I think for so many changes and breaking APIs it's important to have a clean history.
It might even make sense to review the "users" of that new API first, to get a feeling how this API really feels like: