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

Fix streaming interface to parseCSV #342

Merged
merged 11 commits into from
Aug 14, 2023
Merged

Fix streaming interface to parseCSV #342

merged 11 commits into from
Aug 14, 2023

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Aug 12, 2023

Summary

This PR adds a couple of tiny fixes to support CSV streaming, plus a couple of unit tests to show the expected usage.

Details

There were a couple of wee problems with the streaming API:

  1. Expand references was trying to expand streams. Naughty.
  2. You can't pass response.body straight into csv-parse. response.body is a browser-compliant ReadableStream, but we need it to be a node Readable. Subtle. All we have to do is call Readable.from to convert the stream to a compatible API
  3. The callback was always invoked after the final chunk - even if it's passing an empty chunk. Not a big deal but I thought I'd drop a fix.

There are tests in index.test.js which show streaming working from the filesystem and from unidici request. I have also tested this without a mock (ie, host a csv file locally with npx serve, pull it down with node native fetch, and parse it) - but obviously there's no use commiting that. I am satisfied that the mock exercises the same API.

@mtuchi I think you should probably merge this into your msgraph work for akf. You should be able to stream freely from there.

@josephjclark josephjclark changed the title Fix streaming Fix streaming interface to parseCSV Aug 12, 2023
Copy link
Collaborator

@mtuchi mtuchi left a comment

Choose a reason for hiding this comment

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

Thanks @josephjclark this looks good 👍🏽 , I have reviewed and tested the implementation,
Tiny changes i added import {Readable} from 'node:stream' in Adaptors.js Since it's being used in parseCsv and the job was failing when i tried to use parseCsv
and i have remove import {Readable} from 'node:stream' in test.js since it was not being used.

@mtuchi mtuchi merged commit d44129e into main Aug 14, 2023
@mtuchi mtuchi deleted the fix-streaming branch August 14, 2023 09:09
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