undefined vs null is inconsistent for nullable columns

[echentw]

There seems to be a bug in mammoth where Mammoth typing is telling you that the type of a field in a row result is undefined, when in fact the value is null.

Here's my minimal reproduction, let me know if I'm misunderstanding something or made a mistake!

So let's say I have a table defined:

const person = defineTable({
    name: text().notNull(),
    age: integer(),
});

And now I want to select some rows from this table:

const results = await db.select(db.person.name, db.person.age).from(db.person);

My IDE is telling me the inferred type of results is {name: string, age: number | undefined}[]. However, when I console.log(results), I notice that I actually get null (rather than undefined) values for age.

[echentw]

It would be nice to have the column field be optional when I'm inserting into the table, but maybe the query result types should be explicitly T | null to match the postgres format? I'm not sure how hard this is to do.

[martijndeh]

So, regarding the type of the column in the select row, you are right. Mammoth defaults to undefined and not to null or null | undefined. Personally, I generally try to treat null and undefined the same, but you're right this is just wrong in Mammoth right now. Just to understand completely: how is this an issue for you?

Regarding the insert in your later comment: when inserting you should be able to ignore the nullable columns e.g. db.insertInto(db.person).values({ name: 'Eric' }). Is this not working for you?

[echentw]

I personally treat undefined and null differently, e.g. I often have code that looks like

if (variable !== null) {
  // do something
}

or

if (variable !== undefined) {
  // do something
}

I think this makes sense because null and undefined have some semantic differences in javascript (e.g. whether a key exists on an object vs that key's value being explicitly set to null).

I can see this being a bigger problem down the road if I'm working with a larger team and we're passing mammoth row objects around functions.

export type Person = {
  name: string;
  age?: number;
};

const person = defineTable({
    name: text().notNull(),
    age: integer(),
});

...

const results: Person[] = await db.select(...).from(db.person);

...

function canVote(person: Person) {
  if (person.age === undefined) return false;
  return person.age >= 18;
}

This code would be wrong and can't be type-checked, and the dev who writes the db code might not necessarily be the same dev who writes the canVote code.

I'm currently working around this by explicitly setting all null values returned from a mammoth query to be undefined (so that the actual value reflects what Typescript thinks the type is). Ideally I'd like to not have to do this.

Re: the insert part in my later comment:
This isn't an issue for me. I think it's nice that it works that way!

[martijndeh]

In your opinion, what should the type be of a nullable column's value in a row?

[cakoose]

[@echentw and I are working together on a project that uses Mammoth.]

I think the most important thing is for the TS types to be correct. That way I'll get a compile error if I mess something up.

Beyond that, I have a preference for using JS null for database null values. Why: JS has two ways to represent a "missing" value: null and undefined. For consistency, I try to always use null for my own types and only use undefined for the special cases where JS forces it on you, e.g. Map.get().

So for the person example, these are my ideal types:

type PersonSelect = {
    name: string;
    age: number | null;
};
const results: Array<PersonSelect> = await db.select(db.person.name, db.person.age).from(db.person);

type PersonInsert = {
    name: string;
    age?: number | null
}
const newValue: PersonInsert = { ... };
await db.insertInto(db.person).value(newValue);

[martijndeh]

That's interesting. I've also seen codebases where null is avoided and undefined is used instead. There is even a specific case in eslint's eqeqeq rule to warn when doing strict equality on null values (so it matches for undefined as well). Also, some authors try to avoid null altogether, see sindresorhus/meta#7.

Obviously, it all depends on what the query executor function will return. As this is user land I guess it makes sense to at least be able to configure whether you're using null or undefined or even both.

[cakoose]

Yeah, there are a few strategies to dealing with the null/undefined confusion:

1. Allow null and undefined to be used interchangeably (and used together on the same type) and have all your utility functions treat them the same way.
2. Try and stick with one -- I happen to prefer null but either one seems fine.

Configurability would be great! (A type parameter to defineTable defineDb, or something like that?)

[echentw]

To me, the problem isn't the non-configurability, but rather just that Mammoth is giving me a null value while saying that the type is undefined.

Oo, a possible solution is to change the type to null | undefined? That way Mammoth is being explicit that it's treating null and undefined in the same way?

[cakoose]

BTW, here's an unsafe workaround to convert all undefined/optional fields to nullable fields:

type OptionalToNullable<O> = {
    [K in keyof O]-?: undefined extends O[K] ? NonNullable<O[K]> | null : O[K];
};

// Unsafe function, use carefully.
export function optionalToNullable<T>(x: T): OptionalToNullable<T> {
    return x as OptionalToNullable<T>;
}

Edit: Ended up expanding it a bit:

type NonOptional<T> = T extends undefined ? never : T;
type OptionalToNullable<T> = {
    [K in keyof T]-?: undefined extends T[K] ? NonOptional<T[K]> | null : T[K];
};

type NonNullable<T> = T extends null ? never : T;
type NullableToOptional<T> = {
    [K in keyof T]: null extends T[K] ? NonNullable<T[K]> | undefined : T[K];
};

// Unsafe functions, use carefully.  Mammoth thinks nullable columns are returned as optional
// fields, but they're actually nullable (https://github.com/Ff00ff/mammoth/issues/183).
// Use this to convert a record from optional fields to nullable fields.
export function optionalToNullable<T>(x: T): OptionalToNullable<T> {
    return x as OptionalToNullable<T>;
}
export function nullableToOptional<T>(x: T): NullableToOptional<T> {
    return x as NullableToOptional<T>;
}

Still very tedious to actually use them on every query/response from the DB...

[cakoose]

@martijndeh: What is your opinion on using null instead of undefined to represent Postgres NULLs? I think that matches what the pg module does.

Tangent: I tried experimenting with changing it to null in the Mammoth codebase but didn't make much progress. One weird thing I noticed:

1. This test in select.check.ts selects id (non-null uuid) and value (nullable integer): link.
2. The expected type snapshot is {id: string; value: number}[]: link. I expected something like {id: string, value?: number}[] or {id: string, value: number | undefined}.

[martijndeh]

Personally, I prefer never to use null and always use undefined. I make no semantic difference between undefined and null. This simplifies a lot of code in my opinion.

But, because pg defaults to null, and the query executor is in user-land, it's more straightforward to accept null. In Mammoth I don't want to do anything with the resulting rows as it may negatively impact performance. So I think null should be the new default. Would be nice to be able to switch back to undefined but that's optional.

Code wise I'm hoping most can be fixed by changing ResultSetDataType which is setting the undefined. For the rest there are probably some functions on the expressions which need to accept null as well as undefined.

This will probably be a breaking change so version 2.0 is coming.

Lastly, regarding the snapshots, yes so the snapshotting has difficulty with determining whether something is T or T | undefined (I don't know if the same applies for T | null). To fix this there is a snapshot helper. This basically passes in GetDataType<T, boolean> instead of just T (or T | undefined) which allows to test the required vs optional in the snapshots. In the test you're checking coincidentally this snapshot helper is not used.

[cakoose]

One thing that might become less confusing after switching from undefined to null: Doing a UPDATE to set a field to NULL:

await db.update(db.t1).set({a: 'hello', b: undefined});
await db.update(db.t1).set({a: 'hello'});

I think the first statement sets b to NULL and the second one doesn't modify b. This is somewhat error-prone because while TS and JS allow you to distinguish between these two cases, they also make it easy to confuse them.

(For SELECT and INSERT this subtlety doesn't matter.)

[martijndeh]

I agree it's error prone. TypeScript 4.4 is addressing this through exact optional property types though. https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#exact-optional-property-types.

[cakoose]

Yeah, that should help.

However, JS/TS programmers are used to undefined being roughly equivalent to a missing field in many contexts. Even if TS introduces types to distinguish, I think it might be hard to change that way of thinking.