Skip to content

Conversation

piboistudios
Copy link

@piboistudios piboistudios commented Nov 22, 2022

Hi. This library works quite well other than the fact that in the browser, the AsyncGenerator does not work.

This is because encoder.encode checks each part, and if the part is a File, it expects part.stream() to be a node stream.Readable.

This patch will use a duck-type check to see if it is a ReadableStream by checking for presence of a getReader method and then yielding values from the reader. It falls back to the original behavior otherwise.

ETA: There was also a small eslint issue that needed to be resolved.

@octet-stream
Copy link
Owner

Hi. Thanks for your PR! I will take a look.

@octet-stream
Copy link
Owner

This is because encoder.encode checks each part, and if the part is a File, it expects part.stream() to be a node stream.Readable.

Not really. I do expect a File.stream to return async iterable, no matter whether it Node.js Readable stream, or WHATWG ReadableStream, or if it has other source for data.

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #13 (9a09de6) into master (b9a4a33) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #13   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          541       584   +43     
  Branches        76        81    +5     
=========================================
+ Hits           541       584   +43     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/FormDataEncoder.ts 100.00% <100.00%> (ø)
src/util/getStreamIterator.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@octet-stream
Copy link
Owner

octet-stream commented Nov 22, 2022

I did small code review. Other than the things I mentioned, everything else is good.

@octet-stream octet-stream added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Nov 22, 2022
@piboistudios
Copy link
Author

I did small code review. Other than the things I mentioned, everything else is good.

Hi, I will make the aforementioned changes hopefully sometime today if not Friday (holiday here tomorrow).

@octet-stream
Copy link
Owner

You know what? I'll probably just merge it as it is and do everything myself. Or do those changes in PR, whatever. Thank you for your participation! And happy holidays :)

@octet-stream
Copy link
Owner

Reverted your ESLint fix, because I don't see any errors both on my local machine and on GitHub action.

@octet-stream
Copy link
Owner

Also, added a test case for ReadableStream objects that does not implement Symbol.asyncIterator.

Copy link
Owner

@octet-stream octet-stream left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@octet-stream octet-stream merged commit d97a405 into octet-stream:master Nov 24, 2022
@octet-stream
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants