Skip to content

fix(blob, file, tests): Minor improvements for Blob and File implementations #120

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 10 commits into from
Oct 24, 2021

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Sep 27, 2021

The purpose of this PR is:

This PR includes minor improvements and fixes for File and Blob internals

This is what had to change:

  • Blob.stream().cancel() triggers iterator.return() on underlying source;
  • Improve type casting for FilePropertyBag.lastModified to match browsers behaviour (This behaviour tested in Chrome, Firefox and Safari), now NaN values will be interpreted as 0 rather than Date.now();
  • Improve errors reported by Blob constructor;
  • Move Blob.slice() implementation outside the Blob class and implement it as generator function. Since blob treats iterable objects as a sequence and behaviour looks the same;
  • Apply private values as we iterate over blob parts.

This is what like reviewers to know:

This PR is more of a housekeeping with minor changes only. This also changes one thing in terms of File behaviour, just to match File implementations in browsers.


  • I prefixed the PR-title with docs: , fix(area): , feat(area): or breaking(area):
  • I updated ./CHANGELOG.md with a link to this PR or Issue
  • I updated the README.md
  • I Added unit test(s)

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             main      #120   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         4           
  Lines           ?       434           
  Branches        ?        73           
========================================
  Hits            ?       434           
  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 0e07457...5e57899. Read the comment docs.

@octet-stream
Copy link
Contributor Author

Ah, same error for blobs and files backed by fs. Maybe you just run tests with -s flag? I think this should fix this errors :)

@octet-stream
Copy link
Contributor Author

@jimmywarting @LinusU can somebody review this one, please?

@jimmywarting
Copy link
Contributor

also could you test npm run test-wpt manualy also, there is no CI job for it yet...

@octet-stream
Copy link
Contributor Author

also could you test npm run test-wpt manualy also, there is no CI job for it yet...

That's what I do locally :)

@jimmywarting jimmywarting merged commit 2c11647 into node-fetch:main Oct 24, 2021
@octet-stream octet-stream deleted the minor-fixes branch October 25, 2021 05:13
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