Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: storeDirectory accepts iter of file objects #1924

Merged
merged 3 commits into from
May 19, 2022

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented May 18, 2022

Motivation:

@gobengo gobengo force-pushed the storeDirectoryFileObject branch from 3834c60 to 4aac6a2 Compare May 18, 2022 21:45
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 18, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a668f71
Status: ✅  Deploy successful!
Preview URL: https://beaff0aa.nft-storage-1at.pages.dev

View logs

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #1924 (a668f71) into main (27394f6) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #1924   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1269      1259   -10     
=========================================
- Hits          1269      1259   -10     
Impacted Files Coverage Δ
src/lib.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27394f6...a668f71. Read the comment docs.

@gobengo gobengo marked this pull request as ready for review May 18, 2022 22:11
@gobengo gobengo changed the title feat: store directory file object feat: storeDirectory accepts iter of file objects May 18, 2022
@gobengo gobengo requested a review from yusefnapora May 18, 2022 22:12
Copy link
Contributor

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

Looking good - if you haven't tried just passing FileObjects through maybe give it a go... it seems like it should work but I don't have time to mess with it atm

stream: () => AsyncIterable<any>
}

export type FilesSource = Iterable<File>|Iterable<FileObject>|AsyncIterable<File>|AsyncIterable<FileObject>
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely worth adding the type alias 😄

const file =
fileObject instanceof File
? fileObject
: new File(await all(fileObject.stream()), fileObject.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to await the stream here? It would be cool if we could just pass in [fileObject.stream()]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that toImportCandidate inside of encodeDirectory will accept anything with a stream() method that returns an async iterable. So you may be able to skip the coercion to File here and just pass everything through.

Unless you tried that already and it didn't work, lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't so simple. I didn't want to augment more functions than needed, but insofar as somethings were typed as wanting File, this required buffering. afaict constructing a dom File requires not just a stream but a fully-read Uint8Array.

I just pushed a change though that allows the FileObject to encodeDirectory and makes this code less ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

… before passing to encodeDirectory, which now takes a FilesSource
@gobengo gobengo requested a review from yusefnapora May 19, 2022 17:39
Copy link
Contributor

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

beautiful 💯

@@ -758,10 +747,11 @@ const decodePin = (pin) => ({ ...pin, created: new Date(pin.created) })
* the stream is created only when needed.
*
* @param {string} path
* @param {Blob} blob
* @param {Pick<Blob, 'stream'>|{ stream: () => AsyncIterable<Uint8Array> }} blob
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this type definition 😄 - I should start using Pick and Omit more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah :) it would be cool if there was something like an eslint rule that helped you specify your inputs as 'narrowly'/minimally as possible. e.g. in this case we didn't really need a whole Blob, just a subset

*/
function toImportCandidate(path, blob) {
/** @type {ReadableStream} */
/** @type {AsyncIterable<Uint8Array>} */
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah its so cool how node made stream.Readables implement AsyncIterable

@gobengo gobengo merged commit 377b045 into main May 19, 2022
@gobengo gobengo deleted the storeDirectoryFileObject branch May 19, 2022 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants