Closed jimmywarting closed 4 years ago
I almost thing you should remove all async parts and strive towards a synchronous api. then when eg node-fetch iterates all fields it can read all files is a standard way with the new public api's available on blob/files and concatenate all file streams into one stream then it would also figure out the total size by itself also
Hello and thanks for the issue. Sorry for such a late response. I will totally take a look at this. But only thing I'm not planning to do is to remove async API from the module. Because, you know, I want to allow to use this module with clients that does not support it out of the box.
don't support what out of the box?
I meant FormData requests using this library. node-fetch will iterate through the all fields of the FormData instance to make a request, right?
Looking at the fetch spec (where fetch extracts the body)
if it object’s type is FormData it should run the multipart/form-data encoding algorithm, with object as form data set and with UTF-8 as the explicit character encoding.
That encoding algorithm is not defined on either FormData or The Fetch spec. it's just an explanation of what step should be taken when extracting the content. Fetch or any other api (XHR) has no internal/public method it can call on FormData to get the content or the stream.
So i think node-fetch should do the encoding algorithm on there part since it's possible to get all entires form it now. that way we can support any formdata implementation or polyfill
And it's possible that we are going to ignore any entry that isn't a blob, file or a string.
We can figure out the length ourself with blob.size
+ string.length
and also generate a boundary ourself that should also be uniq every time if you where to reuse the same formdata instances
And as you can see in this browser example:
fd = new FormData()
console.log(new Request(fd).headers.get('content-type')) // null
console.log(new Response(fd).headers.get('content-type'))
console.log(new Response(fd).headers.get('content-type')) // it's different
content-type is uniq every time. To me that indicates that fetch runs the encoding algorithm each time
Sorry for a very late response. You know, I think I'll better add support of the third party File-ish (and Blob) objects rather make one in this project. (as you do here) The FormData#entries() method will return it as well as Read, Buffer and Readable fields. So this means that I have to rewrite my pull request to bring form-data encoding algorithm. I mean, why don't we support these classes if they're native for Node? My package support them as fields (form-data support them too as I can see).
So what do you think?
cc @bitinn
So, since v1.8.0 formdata-node support File, ReadableStream and Blob -like objects. I will update my PR at any time soon.
Not sure if I can close this issue.
So what would i expect to get if i did fd.get('file')
? A File with .stream()
?
Yup. I'm using fetch-blob
for tests, so I expect .stream()
method to be in File/Blob object.
But I'm not sure how could I expose filename
(the 3rd parameter of .set() and .append() methods) for Blob, Buffer and streams to make possible to get its value outside of FormData.
I mean, currently you need to set this argument for Blob's in .append() or .set() method separately:
import FormData from "formdata-node"
import Blob from "fetch-blob"
const blob = new Blob(["On Soviet Moon, landscape see binoculars through YOU."], {
type: "text/plain"
})
const fd = new FormData()
fd.set("blob", blob, "file.txt")
fd.get("blob") // Returns the Blob, but it doesn't have any information about filename
const iterator = fd.entries()
iterator.next() // Same here. Returns blob as is, but no filename information
Could you normalize it to a File? like the browser dose when you append a blob and get the same field
import Blob from "fetch-blob"
const blob = new Blob(["On Soviet Moon, landscape see binoculars through YOU."], {
type: "text/plain"
})
const fd = new FormData()
fd.set("blob", blob, "file.txt")
console.log(fd.get("blob"))
> File {
name: "file.txt"
size: 53
type: "text/plain"
}
Yeah, I think that's might work for this case.
And I see that FormData in browsers does not allow to set ReadableStream as a field value. Well, that was my mistake. Or maybe I will allow this, since you still can you ReadStream
(to read files in Node) as well as Readable
stream. I will take a look how can I make this work.
Could you parhaps do something like this:
const file = utils.fileFromFsSync('./package.json')
fileFromFsSync would set the size and patch the .stream()
to return a new readable stream
function fileFromFsSync (path) {
const stats = fs.getStats(path)
const file = new File([], name)
file.size = stats.size
file.stream = () => fs.createReadStream(path)
}
fd.append( 'file', utils.fileFromFsSync('./package.json') )
Or maybe I'll better to allow the 4th optional argument in .append()
and .set()
methods to provide additional information about file? This argument will be only used when filename is set and/or given value is any kind of Readable/ReadStream/Readable stream:
import {createReadStream, statSync} from "fs"
import FormData from "formdata-node"
const file = createReadStream("path/to/a/file.txt")
fd.set("file", file, {size: statSync("path/to/a/file.txt").size})
That way you can decide to use sync or async version of fs.stat()
, or even set this value manually and I will consider this object as a File-like.
For otherwise, I will return streams and Buffer as is, so you can ignore them or consider these objects as plain text? Except you can't know their length (only for ReadStream, since you can actually get this value from fs.stat()
, which also means that you could consider ReadStream as File, but take their size manually in form-data encoding algorithm the way you need)
i would rather prefer that you stick to how FormData IDL works and not create another unconventional 4th parameters.
But sure, the object that you append/set could be a file like object
fd.set("file", {
name: 'file.txt',
type: 'text/plain',
size: statSync("path/to/a/file.txt").size,
stream: () => createReadStream("path/to/a/file.txt"),
// maybe not necessary.
// get arrayBuffer() => Promise.resolve(this.stream)
// get text() => Promise.resolve(this.stream),
// slice: () => {}
// toString: () => '[object File]',
// [Symbol.toStringTag] = 'File',
})
the 3th arugment would almost be useless in node enviorment since nobody would use it and there is no blob. but if they do then you could just replace the obj.name with the 3th argument
I think 4th argument is not necessary for you, since you'll always expect a File-like object returned by FormData#entries()
.
And formdata-node is already support File-like objects, except you still need a constructor with exact name.
But I still think is that necessary to support at least ReadStream somehow, since we're talking about Node.js environment, when there's no standard File objects. I just want to give a way to pass additional information to a field, in my implementation only, or just use some File implementation, which is not necessary for me. I only expect that this will be the object with standard API.
the 3th arugment would almost be useless in node environment since nobody would use it and there is no blob. but if they do then you could just replace the obj.name with the 3th argument
Since I will always return File-like object for Blob, this argument will be used to fill the File#name
property.
I think I can allow to pass plain file with necessary fields and consider this as a File.
Just released 2.0.0 version.
Now FormData#{set,append}()
can take an optional 4th or 3rd argument – options, so you can set additional parameters for streams (aka Readable, ReadStream and ReadableStream):
import FormData from "formdata-node"
import {statSync, createReadStream} from "fs"
const fd = new FormData()
fd.set("stream", createReadStream("path/to/file"), {
size: statSync("path/to/file").size
})
fd.get("stream") // -> File
Note that if you didn't set the size
option, even ReadStream will be returned as-is rather than as a File (I almost thing is that you can't tell its size without using fs.stat
or without reading the whole thing first)
Release note can be found here: https://github.com/octet-stream/form-data/releases/tag/v2.0.0
Create a file class and use this somehow (when appending items).
perhaps blobParts could include a stream if you also provide a known size