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

"this.mapHeaders is not a function" #99

Closed
tjconcept opened this issue Oct 11, 2018 · 12 comments · May be fixed by MilesNorton/developer-exercise#2 or MilesNorton/developer-exercise#3
Closed

"this.mapHeaders is not a function" #99

tjconcept opened this issue Oct 11, 2018 · 12 comments · May be fixed by MilesNorton/developer-exercise#2 or MilesNorton/developer-exercise#3

Comments

@tjconcept
Copy link

  • Operating System: macOS 10.13.6
  • Node Version: 10.11.0
  • NPM Version: 6.4.1
  • csv-parser Version: 2.0.0

Expected Behavior

$ echo "a
> 1" | csv-parser
{"a": "1"}

Actual Behavior

$ echo "a
> 1" | csv-parser
/Users/tj/.npm-config/lib/node_modules/csv-parser/index.js:70
        const newHeader = this.mapHeaders({ header, index })
                               ^

TypeError: this.mapHeaders is not a function
    at headers.forEach (/Users/tj/.npm-config/lib/node_modules/csv-parser/index.js:70:32)
    at Array.forEach (<anonymous>)
    at CsvParser._compile (/Users/tj/.npm-config/lib/node_modules/csv-parser/index.js:69:20)
    at CsvParser._online (/Users/tj/.npm-config/lib/node_modules/csv-parser/index.js:181:12)
    at CsvParser._transform (/Users/tj/.npm-config/lib/node_modules/csv-parser/index.js:242:16)
    at CsvParser.Transform._read (_stream_transform.js:190:10)
    at CsvParser.Transform._write (_stream_transform.js:178:12)
    at doWrite (_stream_writable.js:410:12)
    at writeOrBuffer (_stream_writable.js:394:5)
    at CsvParser.Writable.write (_stream_writable.js:294:11)

How Do We Reproduce?

$ echo "a
> 1" | csv-parser
@shellscape
Copy link
Collaborator

Not sure what scenario would cause this. Our tests don't reflect that, and I'm unable to reproduce using your example. Line 29 in the v2.0.0 tag source (which you're running) sets the options and asserts that mapHeaders cannot be undefined

if (Array.isArray(opts)) opts = { headers: opts }
which is then set on line 37 to the instance
this[key] = options[key]
, and line 69 is using an arrow function
this.headers.forEach((header, index) => {
sothis is from the same scope as the previous line, on which this.headers exists.

Probably something you're going to have to investigate on your end and submit a patch or PR for.

@tjconcept
Copy link
Author

What? It's right here:

mapHeaders: argv.remove ? mapHeaders : null,

@shellscape
Copy link
Collaborator

Sweet. Please open a PR and we'll merge your fix.

@tjconcept
Copy link
Author

I don't have a fix. I have no idea what that code does.

How could yours work? I suppose that's the key to the bug.

@dimfeld
Copy link

dimfeld commented Oct 15, 2018

It looks like bin/csv-parser wasn't updated for the breaking changes in version 2.

@shellscape
Copy link
Collaborator

Happy to merge and publish a PR to fix. Just don't have the time to triage for another week or so.

@tjconcept
Copy link
Author

It looks like bin/csv-parser wasn't updated for the breaking changes in version 2.

There must be something else (or more) at play.

I'm unable to reproduce using your example

I think @shellscape should first triage this so we fully understand the bug before attempting a fix.

@dimfeld
Copy link

dimfeld commented Oct 15, 2018

The root of the problem appears to be where the defaults are merged with the passed-in options.

const options = Object.assign({}, defaults, opts)
where the default and options merge used to be a more manual process performed in a way that would not treat falsy values for the map* functions as valid arguments. (Below code is from the v1.12.1 tag)
this.mapHeaders = opts.mapHeaders || identity

The most obvious fix to me is to just revert back to the old option-parsing code, maybe just for the map functions. If that sounds ok to you, I can probably submit a PR some time this week. bin/csv-parser also needs its mapHeaders arguments updated.

@tjconcept
Copy link
Author

I agree with that analysis, but that doesn't explain why @shellscape is not able to reproduce it. I think that should be debugged first.

@shellscape
Copy link
Collaborator

(On a plane right now and the wifi is miraculously working)

It might be a sticky global bin. My first step in every issue is to try and reproduce locally. If it's unreproducible for me, but reproducible for a bunch of others, then it's probably a wonk on my end. I wouldn't give my lack of reproduction much credence until I've got the time to investigate.

I wouldn't recommend a reversion as a fix, but rather trying to get the new style to party correctly with the breaking changes in the major.

@scottp
Copy link

scottp commented Oct 22, 2018

Part of this may be to do with documentation error. The documentation on NPM shows

      .pipe(csv(
        {
          mapHeader: function(header, index) {

But the actual code is

      .pipe(csv(
        {
          mapHeaders: function(header, index) {

@shellscape
Copy link
Collaborator

Commit a22f006 resolves this. (Github looks to be having issues tonight)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants