Remove files sub #344
Remove files sub #344
Conversation
…-ready drive error)
|
Does this also needs like in watch eg Line 507 in 7d3fbe5 Edit: Ah nop, it uses the encoding already (it's even in the line itself) Then the watch method should do the same I think, passing in the encoding instead of doing it itself |
index.js
Outdated
|
|
||
| if (recursive === false) return shallowReadStream(this.files, folder, false) | ||
| // '0' is binary +1 of / | ||
| return folder ? this.entries({ gt: folder + '/', lt: folder + '0' }) : this.entries() |
There was a problem hiding this comment.
Something to consider, we are saying "if folder then use range, otherwise default" but we are not doing that in watch for example
Can't just assume to always use a range?
There was a problem hiding this comment.
Since we are normalizing all filenames
Or watch should do the same? (explicit condition to use range)
index.js
Outdated
| @@ -172,7 +163,8 @@ module.exports = class Hyperdrive extends ReadyResource { | |||
| await blobsCore.ready() | |||
|
|
|||
| this.blobs = new Hyperblobs(blobsCore) | |||
| this.db.metadata.contentFeed = this.blobs.core.key | |||
| const db = this.db.tree || this.db | |||
| db.metadata.contentFeed = this.blobs.core.key | |||
There was a problem hiding this comment.
Same here about .tree, I think it's ok
There was a problem hiding this comment.
i think the batch needs a getter for the metadata or a static hyperbee method that returns the db from either a batch or bee
There was a problem hiding this comment.
Setter*?
I think we're already in the path of making the Batch as similar as Hyperbee so a setter for the metadata makes sense. Same about adding getHeader method to Batch
So do I add that or we just leverage the getBee helper in Hyperdrive now?
index.js
Outdated
| await this.files.flush() | ||
| return this.close() | ||
| flush () { | ||
| return this.db.flush() |
There was a problem hiding this comment.
Sanity checking: we no longer need to close this upon flushing a batch? Not really understanding why it's no longer needed, as Hyperdrive.batch() does return a new Hyperdrive object
Also sanity checking: it's normal that batch() returns a Hyperdrive object on the same this.corestore instance, without creating a new session? That seems wrong (definitely if we close the passed in Hyperdrive on flushing, as I think we should)
There was a problem hiding this comment.
Just noticed that we conditionally close the corestore in Hyperdrive.close(), not closing it if we're in a checkout or batch, so calling close with the code as-is seems fine, it won't close the corestore in a batch.
Do note that this means that, when we close a Hyperdrive, any existing checkouts will become unusable (because the underlying corestore is closed), but that might be expected.
There was a problem hiding this comment.
I think you're right! The reason I didn't close it is because in main branch I didn't immediately see this.files being closed but it's actually in the close method so my bad!
Also, it should close it because MirrorDrive expects it as well:
https://github.com/holepunchto/mirror-drive/blob/cbd6dea2f175522536cb767f38fc6d23c9712677/index.js#L87
I mean, it does flush but it doesn't close, I think Mathias saw that so flush must close
Rebased from external Hans's branch to debug a bit