zemirco / json2csv

Convert json to csv with column titles
http://zemirco.github.io/json2csv
MIT License
2.72k stars 362 forks source link

fix: consolidate the API of AsyncParser and parseAsync #492

Closed juanjoDiaz closed 4 years ago

juanjoDiaz commented 4 years ago

Closes #468

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 16db2e4a7475c926a3439851d817544395151aba on juanjoDiaz:consolidate_async_parsers into 55aa0c70374def0dafa342d2a122d077eb87d5e1 on zemirco:master.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 3a67571964f994c3fecb0cd8ec301ccbc77b4cd3 on juanjoDiaz:consolidate_async_parsers into 471f5a7a55375a06a66ce4b0438583d719d6db8f on zemirco:master.

juanjoDiaz commented 4 years ago

I'm also considering if it make sense the current API:

or if they should be replaced by something simpler and more aligned with the sync API like

The methods throughTransform and toOutput are just wrappers around pipe and don't really provide anything.

The method promise is very similar to the finished method that has been added to the stream/promise API in node. The only difference is that it can gather the whole JSON in a string and resolve that.

I can see 2 options here: I could move the promise method to either the JSON2CSVTransform so you can do

const csv = await asyncParse.parse(data).promise();

I could add a utils module with a StreamToString class that gathers the string and exposes a promise method that resolves when the stream ends, so the user can do

const streamToString =  new StreamToString();
asyncParse.parse(data).pipe(streamToString);
const csv = await asyncParse.parse(data).pipe(streamToString).promise()

I like the second one better. This PR already includes the ArrayAsStream method wich is basically ArrayToStream. So having StreamToString seems pretty consistent.

Thoughts?

knownasilya commented 4 years ago

I like option 2 as well

juanjoDiaz commented 4 years ago

This should be good to merge.

juanjoDiaz commented 4 years ago

All feedback has been fixed I think.