zemirco / json2csv

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

return defaultValue for null and undefined #543

Open etwillbefine opened 2 years ago

etwillbefine commented 2 years ago

Current behaviour:

const fields = [{ label: 'ID', value: 'id' }];
const transformer = new Transform({ fields });

Converts only undefined to defaultValue (see getProp)

const fields = [{ label: 'ID', value: (row) => row['id'] }];
const transformer = new Transform({ fields });

Converts null and undefined to defaultValue (see here)

knownasilya commented 2 years ago

I'll leave it to @juanjoDiaz to comment, since it might be that way for a reason. Otherwise the code looks fine.

juanjoDiaz commented 2 years ago

Hi all,

The reason for it to be like this is that undefined means that the field was empty while null means that the value null was there. For example, null is a valid JSON value while undefined is not. I've seen endless discussion about whether null and undefined should be considered the same or only undefined should be considered as missing property and null to indicate that the property was set but was just empty. I don't think that we will solve that here 😅

In any case, this breaks the API since we don't know if our users are currently that actually expect to get the null back. So I don't think that we should merge it.

I'd be happy to add a new property useDefaultValueOnNull given that the PR adds the necessary tests and documentation. Also, if we add such property we probably want to do better caching to avoid the extra boolean check on every field of every object potentially affecting performance.

Al alternative is to use a transform to convert nulls into undefineds.

etwillbefine commented 2 years ago

Hi @juanjoDiaz I agree with you that introducing an additional option (useDefaultValueOnNull) makes sense. I'll free some time for it during next week hopefully.

The reason for it to be like this is that undefined means that the field was empty while null means that the value null was there.

This makes sense but it is actually inconsistent. As described when using a function as fieldInfo value the behaviour is different to using the column key as fieldInfo value. So I guess this should then also be unified? That would still be some kind of breaking change but could be merged into the upcoming v6.x version, right?

Al alternative is to use a transform to convert nulls into undefineds.

Imo the better way is to introduce useDefaultValueOnNull as converting null to undefined using a transformer requires us to hook into the object transformer.

Also, if we add such property we probably want to do better caching to avoid the extra boolean check

I'm not 100% sure I get what you mean by caching. Maybe registering different converters with null/undefined check and one with undefined only?