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: new way of creating temporary files backed up by fs #137

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

jimmywarting
Copy link
Contributor

One thing I have been thinking of in node-fetch when we are parsing large multipart payloads with response.formData() is for the possibility to write dose temporary large file to the filesystem in order to not blow up the memory.

But writing them to a place on the disk would also require cleaning it up.
That's why I have now also introduced the new createTemporaryBlob and createTemporaryFile that writes whatever fsPromise.writeFile(data) supports

The purpose of this PR is:

Creating new temporary files to offload some data to the filesystem and cleaning when it's garbage collected

This is what had to change:

I exported two new features createTemporaryBlob and createTemporaryFile in ./from.js

This is what like reviewers to know:

  • Removing / Cleaning up those temporary Blob also depends on the newish FinalizationRegistry. When a blob is GC and you no longer have any references to that blob anymore, then the file will automatically be removed.
  • I have just learned that fsPromise.writeFile(stream) is more superior to stream.pipe(fs.createWritableStream(path)) as fsPromises. writeFile supports more different type of data. It works with string, ArrayBufferView and (async)Iterator so it supports both whatwg streams and node streams (from v15.14.0, v14.18.0)
    • so i'm going to stop using fs.createWritableStream in all my projects... + it can tell when it's finish with a promise.

  • I updated ./CHANGELOG.md with a link to this PR or Issue ( TODO )
  • I updated the README.md ( TODO )
  • I Added unit test(s)

@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@f077471). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main      #137   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         4           
  Lines           ?       511           
  Branches        ?        83           
========================================
  Hits            ?       511           
  Misses          ?         0           
  Partials        ?         0           

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 f077471...a644eab. Read the comment docs.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jun 11, 2022

import { createTemporaryBlob } from 'fetch-blob/from.js'


const res = await fetch(url)

// thinking `response.blob()` should basically do this
const iterable = res.body
let blob = await createTemporaryBlob(iterable, 'text/html') // will be written to the OS temporary directory

blob = undefined // will GC the blob and eventually remove the file from the temporary location on the disk
import { createTemporaryFile } from 'fetch-blob/from.js'

const fd = new FormData()

for await (const part of parseMultipart(payload)) {
  const file = await createTemporaryFile(part.iterable, part.fileName, { type: part.type })
  fd.append(part.fieldName, file)
}

return fd

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jun 11, 2022

Oh btw, it's also possible to stop the writing if you pass a AbortSignal as an option

either via: createTemporaryFile(data, name, { signal, lastModified}) or createTemporaryBlob(data, type, signal)

@jimmywarting jimmywarting requested review from gr2m and LinusU June 11, 2022 21:52
@jimmywarting jimmywarting self-assigned this Jun 11, 2022
@jimmywarting jimmywarting added the enhancement New feature or request label Jun 11, 2022
@jimmywarting
Copy link
Contributor Author

cc @KhafraDev @ronag @tunnckoCore who may all be interested in this.
with regards to this issues:

@tunnckoCore
Copy link

thanks for the ping.

I've spent last few hours digging around File, FormData, and the different parsers. Making the parser just an (async) iterator function seems awesome.

Copy link

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

elegant 👍🏼

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jun 15, 2022

was wondering if createTemporaryBlob should match the signature of Blob's constructor. so you do: createTemporaryBlob(data, { type, signal })

Everything else in ./from.js had blobFrom|blobFromSync|fileFrom|fileFromSync(path, [mimetype])
So i initially try to match ☝️ this style and having type as 2nd argument

createTemporaryFile pretty much match the same as File's constructor

@tunnckoCore
Copy link

createTemporaryBlob(data, { type, signal }) makes sense and looks good, yep.

from.js Show resolved Hide resolved
@tunnckoCore
Copy link

By the way, this createTemporaryBlob(data, { type, signal }) signature will also allow passing more options. Which will be passed from createTemporaryFile.

Thought it a bit deeper for my use-case, I think that allowing options.filename or something like that being a function would be good. Or it can be just a string. Thing is, I want to be able to pass generated filenames or the original filename. Well, not yet sure completely, but will be good addition.

@jimmywarting
Copy link
Contributor Author

If you want to have a filename, then u should use createTemporaryFile instead that returns a File class rather than a Blob so i don't think you should be able to pass in a filename as an option

Also you should be able to create multiple files with the same name

@tunnckoCore
Copy link

tunnckoCore commented Jun 16, 2022

Well, yea, my thinking was wrong. They are temporary files and it doesn't matter what the filename is. I actually implemented it without any changes to the branch.

The thing that I'll make formidable to do by default is to always create temp files, and later give that standard File to the user. If they want to save on disk there will be option that will just write permanent file to non temp user-chosen dir. Simple. Haha.

And i think we can actually introduce it in the v3 and v2, with a bit backward compat file props. Turns out we switch out off of file.type and file.name in v3.. and v2 been more File compat, haha.

All these recent things really lead to a more clear vision of what v4 can be.

@jimmywarting

This comment was marked as outdated.

@mcollina

This comment was marked as outdated.

@jimmywarting

This comment was marked as outdated.

@jimmywarting

This comment was marked as outdated.

@jimmywarting jimmywarting merged commit 20ee08f into node-fetch:main Jul 6, 2022
@jimmywarting jimmywarting deleted the creating-temp-files branch July 6, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants