Typescript "timestamp" fields include null type

[chetbox]

Hi! Thanks for this library, I've found it super helpful.

I've stumbled into an issue using timestamps with Typescript.

The documentation states:

timestamp maps to WardTimestamp|Date|{isEqual: (other: any)=>boolean}

This is great and pretty convenient but in my usage I have found that the generated type is actually null|Date|WardTimestamp|{isEqual: (other: any)=>boolean}.

Here's the most minimal reproduction I could come up with:

$ echo 'type Foo = { bar: timestamp }' | fireward --lang=typescript
...
export type Foo = {
  bar: null|Date|WardTimestamp|{isEqual: (other: any)=>boolean}
}

However, according to the documentation, I expect to see bar without the null type:

export type Foo = {
  bar: Date|WardTimestamp|{isEqual: (other: any)=>boolean}
}

Edit: I'm using Fireward version 1.5.1.

[chetbox]

The same seems to be true for a top-level type alias of timestamp.

$ echo 'type Foo = timestamp' | fireward --lang=typescript
...
export type Foo = null|Date|WardTimestamp|{isEqual: (other: any)=>boolean}

[bijoutrouvaille]

@chetbox thanks for pointing out the discrepancy. This is definitely an oversight in documentation, which needs to be fixed.

The null in the generated type is important, as it guards you from a common edge case that could be very hard to debug. See this issue: https://github.com/bijoutrouvaille/fireward/issues/17.

Thanks also for the compliments. If you have a project that uses Fireward and that you'd like to share, there will soon be a section near the top of the docs that mentions those.

[chetbox]

I see, thanks for this. It makes sense to support the correct types for clients using server timestamps in this way. I'm using Fireward with Admin SDK in a Firebase Cloud Function where, I believe, a non-optional timestamp will ever be null?

Should we support both use cases somehow?

[bijoutrouvaille]

@chetbox unless you explicitly type your rules item as timestamp|null, the |null constraint will appear only in the TS typings. That does open you up to a write-denied error.

Thisissue belongs to a class of issues in Fireward where there's a difference between input and output typing needs. At this point I have not found any appealing solutions. If we can solve that issue in a comprehensive way, then we wouldn't need to split off the Admin typings, at least due to this part.

[chetbox]

Yes, that would be helpful. It would be nice to know that output timestamp types are always WardTimestamp and input types can be Date|Wardtimestamp, for example.

When used with Typescript, we have to cast the data from an unknown type to the type generated by Fireward. Could we generate a separate input type and output types, allowing the use to select which type is appropriate for them in that case? Maybe only when the types differ?

e.g.

type Foo = { bar: timestamp } could generate Typescript type:

// Input type
interface Foo {
  bar: Date|WardTimestamp|{isEqual: (other: any)=>boolean}
}

// Output types
interface FooSnapshot {
  bar: null|WardTimestamp
}
interface FooOutput {
  bar: WardTimestamp
}

or, perhaps, to reduce the cognitive load and breaking changes:

type InputTimestamp = Date|WardTimestamp|{isEqual: (other: any)=>boolean}
type InterimOutputTimestamp = null|WardTimestamp
type OutputTimestamp = WardTimestamp

type Timestamp = InputTimestamp | InterimOutputTimestamp | OutputTimestamp

interface Foo<TimestampT extends Timestamp = InputTimestamp> {
  bar: TimestampT
}

The same approach could be used for other input types that may be different to output types, if there are any.

[bijoutrouvaille]

@chetbox if that solution is seems good to you, then I can recommend a similar one from this issue https://github.com/bijoutrouvaille/fireward/issues/25. Accounting for the timestamp it could look like the example below.

One thing to keep in mind, some small changes are coming up in Fireward, and timestamps will generate this TS type: null|Date|WardTimestamp|WardFieldValue. The new code is already in master, so you can either build from source, or adjust the example a bit once the new version is released.

type WardFieldValue = {isEqual(v: any): boolean}; // this is part of the new Fireward 1.6.x
type Firenum = number | WardFieldValue;

type WardOut<T> =
    T extends object
    ? { [k in keyof T]: WardOut<T[k]> }
    : T extends number
    ? Firenum
    : (WardTimestamp|null|WardFieldValue|Date) extends T
    ? WardTimestamp | Date | WardFieldValue
    : T;

[bijoutrouvaille]

You can have an input type in a similar way:

type WardIn<T> =
    T extends object
    ? { [k in keyof T]: WardIn<T[k]> }
    : (WardTimestamp|null|WardFieldValue|Date) extends T
    ? WardTimestamp | null
    : T;

Then just wrap the Fireward generated types with WardIn<Foo> or WardOut<Foo>.

[chetbox]

I like that this solution could deal with multiple types that differ between input and output, though the ternary types might get a little hard to read. Would the user always have to wrap their type in WardIn or WardOut or could they use the API, as is, without them?

How about something that allows you to select all the valid Fireward types given the situation?

For the Fireward type type Foo = { bar: timestamp } this is generated:

export interface InputTypes {
  timestamp: Date|WardTimestamp
  number: number | ReturnType<typeof firestore.FieldValue.increment>
  // etc. All Firestore supported types can be listed here
}
export interface InterimTypes {
  timestamp: null|WardTimestamp
  number: number
  // etc.
}
export interface OutputTypes {
  timestamp: WardTimestamp
  number: number
  // etc.
}

export type FirewardTypes = InputTypes | InterimTypes | OutputTypes

// The user's custom type
export interface Foo<Types extends FirewardTypes = FirewardTypes> {
  bar: Types['timestamp']
}

For new users the API stays the same:

const foo1: Foo = {
  bar: firestore.FieldValue.serverTimestamp()
}
await doc.set(foo1)

// Also allowed but incorrect
const foo2: Foo = {
  bar: null
}
await doc.set(foo2)

but, for more type safety:

const foo: Foo<InputTypes> = {
  bar: firestore.FieldValue.serverTimestamp() // `null` here is now a type error
}
await doc.set(foo)

I think this achieves the same as what you're suggesting but is more explicit about which types are allowed when. Both approaches work though!

[bijoutrouvaille]

@chetbox that's a clever solution. I like it. Unlike my solution, yours allows for exact control over the types. For example, the WardOut wrap would not handle timestamp|null type correctly.

I'd probably drop the InterimTypes, as there's not a way to distinguish between that and actual input coming in from the database on the web and RN, and also for the sake of simplicity.

I'll play around with it once I get a bit of free time.

[bijoutrouvaille]

@chetbox I pushed the code with your suggestion into the https://github.com/bijoutrouvaille/fireward/tree/io-generics branch. If you are able to build from source, you can use the feature now. Otherwise, I'm going to develop with it for a while to see how well it works.

[chetbox]

Hi! What's the current status of this? I noticed a new release with a change to Timestamps but am not quite sure if it includes any of these changes?

[bijoutrouvaille]

@chetbox this is still not released. I'm close to launching a work project that I've built with the new feature. It has worked out nicely for the most part, but I've also ran into some issues that I'm trying to trace down before publishing.

Also this addition will likely break existing projects, so I may have to bump the major version to reflect this.

[chetbox]

Great, thanks! If I want to try this locally what's the best way? Any chance we can have a beta/alpha on NPM?

[bijoutrouvaille]

It's not at all hard to compile fireward from source. You just need to install the Haskell Stack tool, which is a single binary file, available for download and also on a number of package managers (brew info stack for mac). I can circle back to the beta channel in about a week if you want to wait.

[chetbox]

Great! I think it's easier for us to wait for the beta as we'll want it integrated into our CI.

[bijoutrouvaille]

@chetbox npm install fireward@beta should now install this feature for you. It's not documented in the readme, but it works essentially as you suggested, minus the InterimTypes.

As I said, it has served me well in a medium-sized commercial project, but there is an issue I've not had the time to track down properly. For some deeply nested objects converting from FirewardInput to FirewardOutput forms requires strange backflips. If you come across it and have some useful info, please post an issue.

[chetbox]

Thanks so much! I'm giving it a go and liking it so far.

I like that the default type is permissive include both input and output types.

export type Foo<Types extends FirewardTypes = FirewardTypes> { ... }

👍

The main thing that tripped me up is FirewardInput and FirewardOutput were the opposite way round to what I was expecting.

The generated source says:

export type FirewardInput = /** what you get from DB */ ...
export type FirewardOutput = /** what you send to DB */ ...

But I think of the types as inputs and outputs to the database so I'd expect them to be the other way round.

A question: Why is a timestamp type defined as WardTimestamp | null in FirewardInput? I would assume it always has a value. Is this to support the case you're dealing with server timestamps so there's an interim null value? Currently I'm using ! to tell my code that all the timestamps read from the database are definitely not null.

[bijoutrouvaille]

@chetbox #17 deals with a hard to debug issue in the client. Because Firstore is meant to be used directly by the clients, the defaults should guard against this issue. Also, it is now easy to customize the types to your liking, but on the other hand, debugging this quirk without being aware of it is time-consuming.

When I was thinking of inputs/outputs, I had in mind mostly the software being written. But upon further thought, I'm becoming inclined to agree with you. The types are not dictated by your software but by Firestore, so the input/output typing reflects the database, therefore the naming of them should be consistent with that.

Luckily, refactoring this is easy. I'll try to get to this tonight or tomorrow.

[bijoutrouvaille]

By the way, I've used custom types in my code, and it did not at all impede my flow (I used VS Code). I defined a type NoSentinels and used it just about everywhere in the project. With automatic imports enabled, It was no more difficult than using FirewardInput/Output.

[bijoutrouvaille]

The FirewardInput and FirewardOutput have been swapped in the new beta release.

[chetbox]

I just wanted to say that we've been using this since your release and we've found the new FirewardInput and FirewardOutput types to be helpful. Thanks for your work on this!

[bijoutrouvaille]

Thanks @chetbox for the update. That's great to hear. I have also been using it for a while now without issues, so I'm making plans on soon publishing it in the stable channel.

[chetbox]

Hi! Any news on publishing this as the stable version? We've been using this to great effect for quite a while. 👍🏼

[bijoutrouvaille]

Thanks for the nudge. It's been quite a spell of busywork lately. I plan to release the 2.0 asap.