Support making request with any blob that have stream() method

[jimmywarting]

This makes node-fetch a bit less dependent on our own Blob implementation.
Now it can construct a Request of any blob like object that has blob.stream()

[jimmywarting]

was the invalid header thing something that came along with the newer version of node?
the test passes in some versions

[bitinn]

yeah I need to fix that issue (on the other hand, if you got time, PR welcomed)

The issue is likely with test/server.js, it's blocking another PR too.

#598 (comment)

[bitinn]

Regarding this PR: I think we should just check that incoming Blob implementation provides API that we will use, not every features of Blob

[jimmywarting]

  return obj && typeof obj.stream === 'function' &&
         /^(Blob|File)$/.test(obj[Symbol.toStringTag])

Good enough? Check if type exist also? we are using that...

---

Eventually we maybe want to accept reading whatwg stream also, perhaps then we should use the async iterator that works on both node and whatwg streams. (just an idea for the feature)

for async (chunk of blob.stream()) {
  dest.write(chunk)
}
dest.end()

[bitinn]

@jimmywarting

• I think size, type, constructor.name checks are needed.
• Should we call stream when arrayBuffer is available? Whichever we use, check it too.
• The rest seems overkill.

[jimmywarting]

Should we call stream when arrayBuffer is available?

I think so.

Blob's has a internal get stream

Both the fetch and FileReader spec says if you want to read/post some blob you should initiate the internal get stream method. even when you use fr.readAs[Arraybuffer|text|DataURL] and also even the blob[text|arrayBuffer|stream] should initiate the "get internal stream" method

it would also mean you would handle memory more efficient and you can start sending chunks of data already to your request.

It would be possible to see someone (maybe even Node themself) doing this eventually:
(Very roof sketch with error/missing parts)

const internalMethods = new WeakMap()

class CustomBlob {
  constructor (...args) {
     // something
  }

  get size () {
    return internalMethods.get(this).size
  }

  stream () {
    return internalMethods.get(this).getStream()
  }

  async text () { return consumeStream(this) }
  async arrayBuffer () { return consumeStream(this) }

  // create a new blob but use the same `getStream()` method
  // and just change the start/end and type
  slice (start, end, type = same) {
    // clone internals (the get Read method)
    const internal = Object.assign({}, internalMethods.get(this))
    internal.options.start = start
    internal.options.end = end
    internal.size = newSize

    const blob = new Blob([], { type: type || this.type })
    internalMethods.set(blob, internal)
    return blob
  }
}

fs.getBlob = function (path, options = {}) {
  const stats = fs.statSync('2gb of pi first numbers.txt')
  const size = stats.size
  const type = getType(path)
  const blob = new Blob([], { type: type })

  // Set the internal get stream method
  internalMethods.set(blob, {
    size: size,
    options: options,
    getStream () {
      return fs.createReadStream(path, options)
    }
  })

  return blob
}

This is something i hope @octet-stream would implement in his formdata-node when appending files to FormData

[bitinn]

@jimmywarting

it would also mean you would handle memory more efficient and you can start sending chunks of data already to your request.

• In theory, yes.
• In practice, I am wondering how likely people choose Blob.stream over plain stream.

Either way, it appears using Blob.stream doesn't make our implementation harder, so I have no issue with it.

Will check into test coverage issue when we fixes the node 12 problem.

[bitinn]

@jimmywarting Just wondering, do you plan on adding some test cases and move this forward?

I have triggered a travis rebuild, hopefully it passes and shows some coverage data.

[jimmywarting]

There are some existing test for Blobs already, I have just changed the way it reads them...
Any particular test you would like me to write?

[bitinn]

@jimmywarting make sense, are there any third-party Blob implementation we would like to test just to be sure?

[bitinn]

Another question:

After merging this PR, Do you think we should publish this Blob implementation as separate package so that #589 and #392 is solved?

(Also remind me to update the readme a bit to tell people we now accepts third-party Blob.)

[jimmywarting]

Are there any third-party Blob implementation we would like to test just to be sure?

Not that i know of that supports the stream.

Do you think we should publish this Blob implementation as separate package

I do think it should be a separate package, But not to happy about making yet another npm-package that dose the same thing with a twist. maybe make a PR to any of this?

• node-blob
• blob.js
• blob-polyfill
• node-file-api

I have helped @eligrey integrated the new read methods on his blob implementation that blob.js and blob-polyfill is based on but they use a older version

[bitinn]

@jimmywarting node-blob seems like the right candidate, because it's actually just the same implementation without our recent update, and it's the only one with explicit nodejs support.

https://github.com/shaneharris/node-blob/blob/master/index.js

[bitinn]

Also how does blob.js work in node? It seems to take a WHATWG stream?

[jimmywarting]

Also how does blob.js work in node? It seems to take a WHATWG stream?

yep, It tries to use WHATWG streams if it exist.
it also assigns blob to the global scope. in that since it's not modular. with it you get FileReader, Blob And File.

I have not tested blob.js in node yet.
It feels a bit to browser specific

[bitinn]

I don’t think it will work at all (also likely not their design goal) (From my phone)

…

On Apr 29, 2019, at 18:23, Jimmy Wärting ***@***.***> wrote: Also how does blob.js work in node? It seems to take a WHATWG stream? yep, It tries to use WHATWG streams if it exist. it also assigns blob to the global scope. in that since it's not modular. with it you get FileReader, Blob And File. I have not tested blob.js in node yet. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[bitinn]

rebased and merge in #629