Add support for null property values

[olivierwilkinson]

closes #142

New nullable function provides an API for defining nullable properties
as well as nullable relations.

The arguments passed to nullable are identical to those passed to
factory when the property is not nullable. This means nullable should
feel natural to use for users familiar to the project.

For example to define a nullable property the user passes the factory
function they would use to normally define the property to nullable
when defining the db. This matches the primaryKey method as a bonus.

To define a nullable relation they would pass the oneOf or manyOf
function with the model name directly to nullable.

The model API methods are all typed to know when it's possible for a
property to be null, and an invariant violation is raised for js users
who try to update a non-nullable property to null.

[kettanaito]

Thank you for opening this, @olivierwilkinson!

We should discuss the initial/nullable values in model definitions in general, there's room for improvement. The main points I'd focus on are:

1. Should the properties unspecified when creating an entity be automatically added using value getters from the model?
2. How to define nullable properties?
3. How to declare and annotate explicit null? When is this the case?

[olivierwilkinson]

Thank you for opening this, @olivierwilkinson!

We should discuss the initial/nullable values in model definitions in general, there's room for improvement. The main points I'd focus on are:

1. Should the properties unspecified when creating an entity be automatically added using value getters from the model?

2. How to define nullable properties?

3. How to declare and annotate explicit null? When is this the case?

No problem, and thanks for the rapid response!

Yeah I agree there are more general purpose ways of defining nullable properties.

I was toying with an API similar to primaryKey, something like:

const db = factory({
  account: {
    id: primaryKey(String),
    firstName: nullable(String),
    lastName: nullable<string>(() => null),
    address: {
      street: String,
      houseNumber: nullable(Number),
      livesWith: nullable(manyOf("account")),
    },
    referrer: nullable(oneOf("account")),
  },
});

Where properties that are omitted during creation use the factory function passed to nullable for the initial value.

I'm not sure of the case where you want a null and only null value, but it could be achieved with:

alwaysNull: nullable(() => null)

Is this closer to what you had in mind? I opted for the minimal solution with this PR as I wasn't sure of the corner cases.

I may have a go at the above for a bit of fun if there aren't any glaring issues? It might bring up more to talk about, and at the very least would improve my understanding of the library!

[olivierwilkinson]

@kettanaito I did end up implementing the above API. I think it's a much better proposal than my original attempt so I pushed to this branch rather than opening a new PR.

It was a blast getting to grips with everything! I'm looking forwards to hearing your feedback. 😄

[kettanaito]

Won't it have the same effect if we omit this if altogether? It should fall down to the very last result.properties.push(), doing the same thing. Let me know if I'm missing something here.

[olivierwilkinson]

The NullableProperty class gets caught by the isObject util since typeof value === 'object' where value is an instance of NullableProperty returns true. I could've written it as follows though:

if (isObject<NestedModelDefinition>(value) && !(value instanceof NullableProperty)) {
  ...
}

The above is definitely clearer as to why it needs to be there but I kinda like that NullableProperty is being handled explicitly right now. I'm very happy with either so up to you what you prefer 😄 .

There may be alternatives around changing the isObject util to be more specific but I don't know the implications of that.

[kettanaito]

I believe value is not quite any. It should be value: ModelDefinitionValue.

[olivierwilkinson]

Hmm, when I do that I get the following error message:
A type predicate's type must be assignable to its parameter's type.

I think I can tweak the logic to make it work for any value, then the type is valid as any. What do you think about this?

function isPrimitiveValueType(value: any): value is PrimitiveValueType {
  return (
    typeof value === 'string' ||
    typeof value === 'number' ||
    typeof value === 'boolean' ||
    value?.constructor?.name === 'Date'
  )
}

export function isModelValueType(value: any): value is ModelValueType {
  return (
    isPrimitiveValueType(value) ||
    (Array.isArray(value) && value.every(isPrimitiveValueType))
  )
}

I can't think of a case where this would cause a false positive but I could be missing something.

[kettanaito]

Why are we checking for nullable properties first? Would some of them fall under isModelValueType check below?

[olivierwilkinson]

I don't think it would be caught by the isModelValueType check, I put it there because it felt happy next to the instanceof PrimaryKey check haha. I can move it below isModelValueType though if you'd prefer?

[kettanaito]

I think there's a TypeScript version mismatch between the local environment and the CI.

[kettanaito]

Hey, @olivierwilkinson 👋

I've pushed a couple of changes to your remote:

1. nullable is now treated as a relation's attribute. Alongside unique, nullable is a property that modifies the relation's behavior. Such properties are defined as "attributes" in the library. I've adjusted the type generics to reflect that.
2. Added the relational value assertions to the tests where we update nullable relational values (in both success and failure scenarios). We need to be certain that the actual relational value is set to null or preserved when the relation is nullable and not, respectively.

The build and tests pass on local, so I'll switch to look into this CI issue.

[kettanaito]

I've updated the feature branch so now CI will report the versions of different tools (we're mainly interested in typescript and tsc compatibility). Let's see.

[olivierwilkinson]

Hey, @olivierwilkinson 👋

I've pushed a couple of changes to your remote:

1. `nullable` is now treated as a relation's attribute. Alongside `unique`, `nullable` is a property that modifies the relation's behavior. Such properties are defined as "attributes" in the library. I've adjusted the type generics to reflect that.

2. Added the relational value assertions to the tests where we update nullable relational values (in both success and failure scenarios). We need to be certain that the actual relational value is set to null or preserved when the relation is nullable and not, respectively.

The build and tests pass on local, so I'll switch to look into this CI issue.

Very nice! Makes more sense for nullable to live in attributes looking at it now. 😄

Thanks for looking into the CI issue 🙌 I wasn't sure how I was going to fix that haha

[kettanaito]

I think the CI failure is related to the typescript and tsc versions mismatch. Here's what CI reports:

node: v14.18.1
npm: 6.14.15
typescript: @mswjs/[email protected] /home/runner/work/data/data
+└── [email protected] 
-tsc: Version 4.4.4

While locally I have:

typescript: @mswjs/[email protected] /mswjs/data
+└── [email protected]
+tsc: Version 4.3.5

Looks like CI is installing the latest tsc which has backward-incompatible changes with 4.3.5 (validates types differently).

[kettanaito]

@olivierwilkinson, perhaps a silly question, but do we have any runtime checks that a non-nullable property can't be set to null when calling .create/.update? Like throwing an exception.

[olivierwilkinson]

@olivierwilkinson, perhaps a silly question, but do we have any runtime checks that a non-nullable property can't be set to null when calling .create/.update? Like throwing an exception.

Not a silly question at all! We have invariant checks that prevent updating a non-nullable property or relation to null, but none for creation. Would you like me to add some for creation, or do you want me to remove the checks during update to avoid a breaking change?

[kettanaito]

If you have a chance, could you please add a few test cases to the existing create.test.ts that it throws an exception when creating an entity with its non-nullable property set to null?

I think we should be explicit about this, so if the user wants to use null as a value, they must always mark that property as nullable.

P.S. Don't forget to pull the changes first ;)

[olivierwilkinson]

If you have a chance, could you please add a few test cases to the existing create.test.ts that it throws an exception when creating an entity with its non-nullable property set to null?

I think we should be explicit about this, so if the user wants to use null as a value, they must always mark that property as nullable.

P.S. Don't forget to pull the changes first ;)

Haha I won't squash either don't worry 😆

That is all done! 😄

[kettanaito]

Thank you for adding the logic and tests when creating non-nullable properties with null. I've pushed to your branch, adjusting the wording, if you don't mind. It's beneficial to contain the current state in the error message ("a non-nullable property/relation") as well as explain why using "nullable" is necessary (and not necessarily what the user wants).

The changes look fantastic. I'll look into the failing CI and try to align the TS versions so the build is green.

[kettanaito]

Issue: PrimaryKey instance matches the NullableProperty class

It seems that TypeScript is comparing class instances based on their properties/methods. This causes the PrimaryKey instances to match the NullableProperty class (both classes share this.getValue property). This results in primary keys being allowed in nested objects when defining a model, which is a no-op.

One way to solve that is to name those internal properties differently. I'm considering renaming getValue to getPrimaryKeyValue for the PrimaryKey class to prevent false positive matches then type-validating.

[kettanaito]

Okay, so it wasn't a TS versions mismatch, in the end, it was a regression in primary key check. Once I pushed the fix, the CI is back to normal now.

[kettanaito]

A hard and definite "approved". You've done an incredible job bringing nullable support to Data! Thank you, @olivierwilkinson, and welcome to contributors! 🎉

We are always looking for collaborators, so if any other issue in Data or MSW ecosystem libraries interests you, do not hesitate to discuss and dive into them. I know we could use your expertise.

[olivierwilkinson]

Thank you for adding the logic and tests when creating non-nullable properties with null. I've pushed to your branch, adjusting the wording, if you don't mind. It's beneficial to contain the current state in the error message ("a non-nullable property/relation") as well as explain why using "nullable" is necessary (and not necessarily what the user wants).

Great! No worries at all, I was struggling to figure out a concise way of phrasing it and I like what you changed it to 😄

[olivierwilkinson]

Issue: PrimaryKey instance matches the NullableProperty class

It seems that TypeScript is comparing class instances based on their properties/methods. This causes the PrimaryKey instances to match the NullableProperty class (both classes share this.getValue property). This results in primary keys being allowed in nested objects when defining a model, which is a no-op.

One way to solve that is to name those internal properties differently. I'm considering renaming getValue to getPrimaryKeyValue for the PrimaryKey class to prevent false positive matches then type-validating.

Oh wow it hadn't occured to me that's how they are comparing them, good to keep in mind! It makes sense I suppose since before your change both NullableProperty and PrimaryKey could be described by the same interface so a PrimaryKey instance does indeed extend NullableProperty. Nicely found 🙌

[olivierwilkinson]

A hard and definite "approved". You've done an incredible job bringing nullable support to Data! Thank you, @olivierwilkinson, and welcome to contributors! 🎉

We are always looking for collaborators, so if any other issue in Data or MSW ecosystem libraries interests you, do not hesitate to discuss and dive into them. I know we could use your expertise.

Amazing 🥳

Ah I'm happy to hear that! I'm keenly interested so I would love to continue contributing where I am able to.

Thank you for putting so much of your own time and effort into reviewing and contributing to make it so polished. I'm really happy with the end result so I appreciate it a lot. This has been a pleasure! 😁

I'm going to go make some nullable properties now 💥

[kettanaito]

All thanks go to you, really. It's by the power of great contributions that we can deliver quality software, together. Thank you!