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: consolidate the API of AsyncParser and parseAsync #492

Merged
merged 5 commits into from
Nov 9, 2020
Merged

fix: consolidate the API of AsyncParser and parseAsync #492

merged 5 commits into from
Nov 9, 2020

Conversation

juanjoDiaz
Copy link
Collaborator

Closes #468

@coveralls
Copy link

coveralls commented Nov 1, 2020

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@juanjoDiaz
Copy link
Collaborator Author

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

  • fromInput allows you to set the input which can be a stream, an array or a single object.
  • throughTransform allows you to add transforms to the input stream.
  • toOutput allows you to set the output stream.
  • promise returns a promise that resolves when the stream ends or errors. Takes a boolean parameter to indicate if the resulting CSV should be kept in-memory and be resolved by the promise.

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

  • parse(input) which takes an input (stream, array or object) and returns the CSV.

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
Copy link
Collaborator

I like option 2 as well

@juanjoDiaz
Copy link
Collaborator Author

This should be good to merge.

README.md Outdated Show resolved Hide resolved
README.md Outdated
const csv = await json2csvParser.parse(myData).promise();
```

If you want to wait for the stream to finish but not keep the CSV in memory you can use the `stream.finished` utility from Node's stream module.
Copy link
Collaborator

@knownasilya knownasilya Nov 5, 2020

Choose a reason for hiding this comment

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

Can we link to the docs for nodes stream module?

README.md Outdated
.on('end', () => console.log(csv))
.on('error', (err) => console.error(err));

myData.default.forEach(item => asyncParser.input.push(item));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does myData come from in this example, and why does it have default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

myData is the same myData as in all the other examples.. any array.
It doesn't have default. I'll remove that.

@juanjoDiaz
Copy link
Collaborator Author

All feedback has been fixed I think.

@knownasilya knownasilya merged commit bcce91f into zemirco:master Nov 9, 2020
@juanjoDiaz juanjoDiaz deleted the consolidate_async_parsers branch November 9, 2020 21:40
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.

Allow AsyncParser to accept the same inputs as parseAsync
3 participants