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

TypeError: Cannot read property 'trim' of undefined #48

Closed
Fil opened this issue Mar 26, 2019 · 3 comments · Fixed by #54
Closed

TypeError: Cannot read property 'trim' of undefined #48

Fil opened this issue Mar 26, 2019 · 3 comments · Fixed by #54

Comments

@Fil
Copy link
Member

Fil commented Mar 26, 2019

It might happen that CSV files will omit redundant commas, in which case d3.autoType breaks with
TypeError: Cannot read property 'trim' of undefined

This might be a desirable feature, in which case it would be good to trap the error and make it explicit.

However my personal preference would be to have it a bit more fault tolerant and avoid crashing on d3.autoType({ foo: undefined }).

Especially if d3.autoType is made the default for d3.csv (#43).

@Fil
Copy link
Member Author

Fil commented Jul 5, 2019

Bitten by this again…

the simplest fix is to set value = (object[key] || "").trim()
https://github.com/d3/d3-dsv/blob/master/src/autoType.js#L3

but there might be a better way

@mbostock
Copy link
Member

mbostock commented Jul 5, 2019

Hmm. The expectation is that the fields are empty strings, not undefined. We could replace undefined or null with the empty string, but I think this should be considered a bug with how we parse DSV with missing columns: the missing columns should be represented by empty strings, not undefined. You can see the inconsistency with the roundtrip through DSV formatting here.

https://observablehq.com/d/0a81c0d60fb498e7

@mbostock
Copy link
Member

mbostock commented Jul 5, 2019

Probably adding a || "" here:

function objectConverter(columns) {
  return new Function("d", "return {" + columns.map(function(name, i) {
    return JSON.stringify(name) + ": d[" + i + "] || \"\"";
  }).join(",") + "}");
}

Fil added a commit that referenced this issue Jul 21, 2019
Reverses one test, so technically we can say that it breaks the API. However README is silent about this situation, so I don't know.

Fixes #48
@Fil Fil closed this as completed in #54 Jul 22, 2019
Fil added a commit that referenced this issue Jul 22, 2019
Reverses one test, so technically we can say that it breaks the API. However README is silent about this situation, so I don't know.

Fixes #48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants