Skip to content

Conversation

@octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Apr 24, 2021

I was trying to upgrade fetch-blob to the latest version and got the following error when I run my tests:

Details

image

You can also reproduce this from node REPL:

  1. Clone fetch-blob to your local machine
  2. Install dependencies as usual
  3. Open .editor REPL:
  4. Put this code into console, then hit Ctrl+D to run it:
const {createWriteStream} = require("fs")

const blobFromPath = require("./from")

void blobFromPath("./LICENSE").stream().pipe(createWriteStream("/dev/null"))

Tried this on Node v15.13.0 and v16.0.0

Seems like the error appears because you didn't check if the start option present.
Here:

end: this.start + this.size - 1

So when the start option is not set, the result of this operation would be NaN.

I think it also happens here:

size: end - start

This PR should solve the problem.

I was also thinking to just set the start to 0 by default, but I'm not sure if I understand how Node.js will handle ReadStream in this case.

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #85 (22ecc66) into master (cc929f3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #85   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          234       234           
  Branches        37        38    +1     
=========================================
  Hits           234       234           
Impacted Files Coverage Δ
from.js 100.00% <100.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 cc929f3...22ecc66. Read the comment docs.

@octet-stream octet-stream mentioned this pull request Apr 25, 2021
@jimmywarting
Copy link
Contributor

Hmm, kind of expected that start would always be present on BlobDataItem... guess i have missed that or just skipped it entierly.
Would be nice to have it be 0 by default

@octet-stream
Copy link
Contributor Author

Done

@octet-stream
Copy link
Contributor Author

Just noticed it was present in v2.1.1

Details

image

Same function in v2.1.2

Details

image

@octet-stream
Copy link
Contributor Author

Can't find it in from.js history. Maybe you forgot to push this change to github?

@jimmywarting
Copy link
Contributor

Can't find it in from.js history. Maybe you forgot to push this change to github?

Could be

@jimmywarting jimmywarting merged commit 483973c into node-fetch:master Apr 26, 2021
@octet-stream octet-stream deleted the octet-stream/fix-range branch April 27, 2021 14:05
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.

2 participants