Skip to content
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

Allow usage of iterable object in Blob constructor. #108

Merged

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Jul 29, 2021

I've noticed that Blob constructor can accept iterables as blobParts argument. This was tested in Chrome, Firefox and Safari. I also found tests for this case in WPT. See: https://github.com/web-platform-tests/wpt/blob/e7d848ca78a17253dde9d49956bc00ae3ba91c57/FileAPI/blob/Blob-constructor.any.js#L58-L105
This PR fixes how constructor threats iterable objects.

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #108 (fabbd8a) into master (0b02843) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #108   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          411       412    +1     
  Branches        60        60           
=========================================
+ Hits           411       412    +1     
Impacted Files Coverage Δ
index.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 0b02843...fabbd8a. Read the comment docs.

@octet-stream
Copy link
Contributor Author

@jimmywarting tests for blobs backed up by fs failing for some reason, but everything related to this patch seem to be working.

@jimmywarting
Copy link
Contributor

jimmywarting commented Jul 29, 2021

👍

Think I got one idea of how to use WPT test

tests for blobs backed up by fs failing for some reason, but everything related to this patch seem to be working.

The test are a tiny bit racy... that is why it fails sometimes and sometimes it's not

@jimmywarting jimmywarting merged commit c2903ca into node-fetch:master Jul 29, 2021
@octet-stream octet-stream deleted the iterable-blob-parts-in-constructor branch July 29, 2021 13:45
@octet-stream
Copy link
Contributor Author

octet-stream commented Jul 29, 2021

The test are a tiny bit racy... that is why it fails sometimes and sometimes it's not

AVA runs tests concurrently (asynchronous tests) and in parallel (each file runs in its own thread using child_process or worker_threads). I believe they need to be isolated if that the source of the problem. Or you can use an -s flag, so AVA will run tests serially.

This pull request was closed.
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