alberthaff / ngx-papaparse

Papa Parse wrapper for Angular
https://alberthaff.dk/projects/ngx-papaparse/docs/v8
MIT License
90 stars 19 forks source link

Parse Csv to observable #59

Closed rodrigowbazevedo closed 4 years ago

rodrigowbazevedo commented 4 years ago

As mentioned on issue #56 this PR implements a way to return de result as Observable using the parseToObservable() method, it has the same signature as parse().

parse(csv: string|Blob, config: ParseConfig = {}): ParseResult;
parseToObservable(csv: string|Blob, config: ParseConfig = {}): Observable<ParseResult>;
const csv = `a,b,c
d,e,f
g,h,i`;

papa.parseToObservable(csv).subscribe(result => {
    console.log('Parsed Accumulation: ', result.data);
});

// Output
// Parsed Accumulation: [['a', 'b', 'c']]
// Parsed Accumulation: [['a', 'b', 'c'], ['d, 'e', 'f']]
// Parsed Accumulation: [['a', 'b', 'c'], ['d, 'e', 'f'], ['g, 'h', 'i']]

papa.parseToObservable(csv).toPromise(result => {
    console.log('Complete Result: ', result.data);
});
// Complete Result: [['a', 'b', 'c'], ['d, 'e', 'f'], ['g, 'h', 'i']]
coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 90


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/papa.ts 13 16 81.25%
<!-- Total: 13 16 81.25% -->
Totals Coverage Status
Change from base Build 88: -4.2%
Covered Lines: 34
Relevant Lines: 40

💛 - Coveralls
rodrigowbazevedo commented 4 years ago

I was unable to test on error case, it seems that papaparser only throws error reading a File, and PhantomJs don't support the File|Blob api.

I really tried to create a invalid csv text...

alberthaff commented 4 years ago

Hi. Thanks for your PRs.

This one still needs a little work (i should probably highlight that file somewhere :-)). Please add parseToObservable and an example to docs/index.md - WIP can be seen here: https://alberthaff.dk/projects/ngx-papaparse/docs/v5

Regarding the testcase - i'm thinking about moving to headless chrome instead. Do you think it would be able to run in chrome? If so, we could split that test into a separate PR, and i'll be happy to merge that after i have updated the test environment.

rodrigowbazevedo commented 4 years ago

I think now is possible to create the test, after update to Angular 9, it updated PhantomJS too and now supports Blob, it is still not supporting the File class but it's possible to test using Blob.

I did some tests after I wrote the last comment and it worked after update to Angular 9, I'll try to do it again ASAP.

I was thinking and maybe it'll be better to do some changes in the way I did the observable, it's delivering each row async, but I'm concatenating each row to delivery the entire file in the end, I did it to keep the same simple Api, you can see it in the example above. The problem is if you have a huge file, you'll probably have a memory problem.

Maybe will be better if I just delivery each row without accumulate it, what do you think?

alberthaff commented 4 years ago

Cool. Then i'll wait with the headless chrome. I think it's fine if we just test it with a Blob. PapaParse's own tests should cover the remaining tests for that part.

I see your point. One issue i see, is that the user would have to accumulate themselves. And if the observable is converted to a promise, how would that handle? Would you only get the last row/ chunk?

An other solution could be to add a config parameter, to enable/ disable the accumulation behaviour.

alberthaff commented 4 years ago

We need to make a decision on this. The two open PRs are the only thing blocking release of v5. I think it should accumulate the parsed data as a default, but have an option to disable this behaviour.

alberthaff commented 4 years ago

Closing this. Feel free to reopen.