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

return defaultValue for null and undefined #543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

etwillbefine
Copy link

  • treat null and undefined as missing
  • apply same behaviour as in
    if (typeof fieldInfo.value === 'function') {
    const label = fieldInfo.label || fieldInfo.value.name || '';
    const field = { label, default: defaultValue };
    return {
    label,
    value(row) {
    const value = fieldInfo.value(row, field);
    return (value === null || value === undefined)
    ? defaultValue
    : value;
    },
    }
    }

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
Copy link
Collaborator

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

@juanjoDiaz
Copy link
Collaborator

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
Copy link
Author

etwillbefine commented Oct 1, 2021

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?

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

Successfully merging this pull request may close these issues.

None yet

3 participants