mafintosh / csv-parser

Streaming csv parser inspired by binary-csv that aims to be faster than everyone else
MIT License
1.41k stars 134 forks source link

strict mode emit error #194

Open basaran opened 3 years ago

basaran commented 3 years ago

Expected Behavior

Not sure

Actual Behavior

It appears, a range error event is emitted when strict mode is enabled, which can be listened with the on("error") as usual.

However, when this range-error event is emitted, nodejs never emits the on("end") and on("close"). I'm assuming this has something to do with the transform being broken. This doesn't affect the process most of the time, file executes and exits fine but if you need to do cleanup, it could be problematic as it was in my case, where I needed to disconnect the db once the readstream has finished.

Changing the emitted event from error to anything worked and also fired the end and close. Please the sample file and script below to produce the issue. Although I'm not positive if the way it is now is the expected way.

Perhaps something is needed in the transformer to handle this, if not and it's okay, I recommend changing the error to line-error since this is not really a stream related error.

       if (!skip && this.options.strict && cells.length !== this.headers.length) {
            const e = new RangeError("Row length does not match headers");
            this.emit("error", e);

How Do We Reproduce?

head1,head2,head3
a,b,c
x,x,x
e,e
y,y,y
f,f
const csvParser = require("csv-parser");
const fs = require("fs");

const rs = fs.createReadStream("./a.csv");

rs.pipe(
    csvParser({
        strict: true,
    })
)
.on("error", () => {
    console.log('caught');
})
.on("end", function () {
    console.log("All the data in the file has been read");
})
.on("close", function (err) {
    console.log("Stream has been destroyed and file has been closed");
});
james4388 commented 3 years ago

+1 I was looking at this too. In my case I use for await

const stream = fs
  .createReadStream()
  .pipe(csv({ strict: true }))
for await (const line of stream) {
  // Do something with line
}

how to handle line error in this case?

basaran commented 3 years ago

This should be done through the stream transformer, but as a quick fix, you can change the error emit from the module source to something like:

        if (!skip && this.options.strict && cells.length !== this.headers.length) {
            const e = new RangeError("Row length does not match headers");

            e.meta = {
                cells: cells,
                lineNumber: this.state.lineNumber,
            }; /* → injected _meta information */

            this.emit("line-error", e);
            /* → changed error to line-error so things end, close nicely */
        } else {
            if (!skip) this.writeRow(cells);
        }

and then you can add it to the stream's event handlers with on("line-error", (err) => { console.log('line-error'); }).

Modifying the node_modules is not a good idea, so if you must, I suggest you clone the repository into your project and do changes there, and require from there.