Skip to content

Fix for the 'start' range of BlobDataItem when reading its stream #85

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

Merged
merged 2 commits into from
Apr 26, 2021

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