zemirco / json2csv

Convert json to csv with column titles
http://zemirco.github.io/json2csv
MIT License
2.72k stars 362 forks source link

Typescript defintion #394

Closed WORMSS closed 5 years ago

WORMSS commented 5 years ago

Hello, I know there are a number of typescript issues/pull requests but they all seem to be closed

I noticed in the definition, FieldValueCallback<T> has the signature of

export interface FieldValueCallback<T> {
  (row: T, field: string): string;
}

from my understanding from the readme file, this should be

export interface FieldValueCallback<T> {
  (row: T, field: { label: string, default: string }): string | null | undefined;
}

being able to return null or undefined to take advantage of the default functionality.

I am a little unsure about field, as I am not using it, but I noticed that in the readme file at the same time so I thought I would mention it.

The lines I am referring to are

value: (row, field) => row.path1 + row.path2, // field = { label, default }
default: 'NULL', // default if value function returns null or undefined
juanjoDiaz commented 5 years ago

Thanks @WORMSS

I'll take a look at this!

WORMSS commented 5 years ago

I checked, field is indeed an object inside preprocessFieldsInfo in JSON2CSVBase

const field = { label, default: defaultValue };
...
const value = fieldInfo.value(row, field);

But it just came to me, but this allows you to return a number and boolean and even objects too from my understanding of processValue.

And I have seen you want to remove stringify from your codebase in the future of v5, but it seems it is currently missing from FieldInfo<T> in the current version. I was actually looking for a way to turn it off on certain fields.

juanjoDiaz commented 5 years ago

stringify is missing indeed.

And yes. Potentially you can return anything and json2csv will process it, although the recommended way is to return a string.

You can also have getter with no field property like:

    value: (row) => row.path1 + row.path2,

I'm updating the docs.

So, the correct signature is probably:

    export interface FieldValueCallback<T> {
      (row: T, field?: { label: string, default?: string }): any;
    }

    export interface FieldInfo<T> {
        label: string;
        default?: string;
        value?: string | FieldValueCallback<T>;
        stringify?: string;
    }

I'm just thinking if we want that any or if we want to return to string|number|boolean|null|undefined since it's definitely safer. Not sure. I'll give it some thought and update the types later today. Feel free to share your opinion on the matter.

WORMSS commented 5 years ago

wouldn't it be

export interface FieldInfo<T> {
        label: string;
        default?: string;
        value?: string | FieldValueCallback<T>;
        stringify?: boolean;
    }

and yes, any is technically the correct answer.. Though, after I pressed the 'send' button on the other issue about stringify, I have a horrible feeling, you are talking about removing stringify the "feature of turning objects/arrays into strings automatically", and not stringify the "option, to put quotes around strings".

If you do plan on removing the ability to return objects/arrays since they are not primitives that a CSV could understand, then I would say string|number|boolean|null|undefined would be the way to go.

juanjoDiaz commented 5 years ago

yeah sorry stringify?: boolean too much copy-pasting here 😄

Let's talk about stringify on the other issue.

juanjoDiaz commented 5 years ago

This will be closed by: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/35749