TS: parse with an explicit record type and `columns: function` may error with "Type 'keyof T' is not assignable to type 'ColumnOption<string>'"

[beta382]

Describe the bug

csv-parse v6.2.1, sync API, typescript

When supplying an explicit record type to parse<T>, and when using the columns option with function type, the accepted type of columns does not properly resolve to (record: T) => (keyof T)[]. It instead resolves to (record: string[]) => string[] (there are other types in the union, which I've excluded for brevity).

It appears that the intention is for it to resolve to (record: T) => (keyof T)[], given the logic here and here. However, the type arguments are not passed through here. This means that Options["columns"] will always default to T = string[], and thus the ternary linked to yields string.

This can cause TS compile failures if keyof T cannot be trivially coerced to string (e.g. if they're not string, or if they're coming from some other generic type).

To Reproduce

Here is a minimal TS example. The expectation is that it should compile.

test("should compile", () => {
  interface Foobar {
    1: string;
    2: string;
  }

  const columnMapping = new Map<string, keyof Foobar>([
    ["col_1", 1],
    ["col_2", 2],
  ]);

  const records = parse<Foobar>('col_1,col_2\n"foo","bar"', {
    columns: (header) =>
      header.map((column) => columnMapping.get(column)),
  });

  expect(records).toEqual([{ 1: "foo", 2: "bar" }]);
});

Instead it errors with:

Type '(header: string[]) => (keyof Foobar | undefined)[]' is not assignable to type 'true | ColumnOption<string>[] | ((record: string[]) => ColumnOption<string>[])'.
  Type '(header: string[]) => (keyof Foobar | undefined)[]' is not assignable to type '(record: string[]) => ColumnOption<string>[]'.
    Type '(keyof Foobar | undefined)[]' is not assignable to type 'ColumnOption<string>[]'.
      Type 'keyof Foobar | undefined' is not assignable to type 'ColumnOption<string>'.
        Type '1' is not assignable to type 'ColumnOption<string>'.

Workaround

(differs slightly based on the specific scenario)

const records = parse<Foobar>('col_1,col_2\n"foo","bar"', {
  columns: (header) =>
    header.map((column) => columnMapping.get(column) as undefined),
});

[wdavidw]

Hi @beta382, not a TS expert, do you have a solution in mind ?

[beta382]

Not a TS expert either unfortunately. I took a quick stab at this, but ended up running into a wall of needing to understand the internals more than I can commit to. Once the seemingly-obvious solution was attempted, some of the tests started to have TS whine about them violating type safety (which is probably right, as many nuances get masked when everything in Options gets assumed to have T = string[]).

At minimum, I'm pretty sure that the type parameters from e.g. OptionsWithColumns and Options ought to be propagated through to Options and OptionsNormalized respectively.

I'm also pretty sure that the type here (the record parameter for the columns callback) should be string[] instead of T. My understanding is that T is intended to be the output row type, but the record parameter for the columns callback is still just an array of strings at that point in the parse (there perhaps is more internals nuance here that I'm missing, but I'm fairly sure T is wrong).

Finally, I think that there is some trickiness that solving this will expose around on_record. What the record parameter for its callback is given differs based on the values given to raw and info (I think, but didn't dig deep enough to verify, that these parameters also influence the shape of the parse output; more trickiness).

[wdavidw]

Two things

1. I agree that record in colums shall be typed string[].
2. I did a quick change to your test replacing 1 and 2 in you interface with a and b and your test validate and run

    it("should compile and run", function () {
      interface Foobar {
        a: string;
        b: string;
      }
      const columnMapping = new Map<string, keyof Foobar>([
        ["col_1", "a"],
        ["col_2", "b"],
      ]);
      parse<Foobar>(
        'col_1,col_2\n"foo","bar"',
        {
          columns: (header) =>
            header.map((column) => columnMapping.get(column)),
        },
        function (err, records: Foobar[]) {
          records.should.eql([{ a: "foo", b: "bar" }]);
        },
      );
    });

[beta382]

Yes, what you've written is able to run because the return type for the columns function is ColumnOption<T extends string[] ? string : T extends unknown ? string : keyof T>[]. Because T = string[] always, this resolves to ColumnOption<string>[] (effectively string[]). The function (header) => header.map((column) => columnMapping.get(column)) then looks like (replacing names with types)

(string[]) => string[].map((string) => Map<string, keyof Foobar>.get(string))
(string[]) => string[].map((string) => keyof Foobar)
(string[]) =>(keyof Foobar)[]

and since keyof Foobar is "a" | "b", it can be coerced to string to satisfy (string[]) => string[]

The reason this fails with the integer keys in my original example is because keyof Foobar is 1 | 2, and so cannot be coerced to string.

A different example that uses string keys (that more closely resembles my actual use case, in which I'm wrapping the parse in a generic function) would be:

it("accepts keyof return values", function (next) {
  function doParse<T>(
    csv: string,
    columnMapping: Map<string, keyof T>,
    expected: T[]
  ): void {
    parse<T>(
      csv,
      {
        columns: (header) =>
          header.map((column) => columnMapping.get(column) as keyof T),
      },
      function (err, records: T[]) {
        records.should.eql(expected);
        next();
      },
    );
  }

  interface Foobar {
    a: string;
    b: string;
  }

  const columnMapping = new Map<string, keyof Foobar>([
    ["col_1", "a"],
    ["col_2", "b"],
  ]);

  doParse<Foobar>('col_1,col_2\n"foo","bar"', columnMapping, [{ a: "foo", b: "bar" }]);
});

(I may be using your testing framework wrong, I'm not familiar with mocha).

---

I took another stab at this, but the complexity and nuances of the parse options put a stop to me. columns = true returns a list of objects each keyed by the header, while columns = false returns a list of strings. info = true wraps the return in an object with info and record keys, while raw = true wraps the return in an object with raw and record keys (and both yields all three keys). This can be further modified by onRecord, which takes in a record (already processed by columns, and respecting info and raw) and potentially performs a transformation (and in some of your tests, even returns mixed types, e.g. here). There's a lot of "shapes" the output can take depending on what options you supply, and capturing that accurately in TS seems like it would be a grand undertaking. All of this complexity is currently hidden because OptionsWithColumns["columns"] is using the default T = string[] for Options.

I've ended up working around the issue for my use case by pre-processing the CSVs to normalize their headers, and then just calling parse<T>(csvString, { columns: true }).

I'm not sure solving this issue will be worth the effort.

[wdavidw]

I sort of agree, the flexibility provided by the wealth of options is hard to express in TS and requires expert TS skills. Let's close the issue ?