bijoutrouvaille / fireward

A concise and readable language for Firestore security rules, similar to Firebase Bolt.
MIT License
238 stars 4 forks source link

Adding FirebaseFirestoreTypes.FieldValue to number types #25

Open ninjz opened 4 years ago

ninjz commented 4 years ago

I've come into a requirement where I am needing to use FirebaseFirestoreTypes.FieldValue.increment() to increment a value within my document. I use this library to help generate TypeScript types which I use within a wrapper function to enforce the type constraints through one centralized function.

Like this:

// Generated TS Type from Fireward
export type Stats = {
  allTime: {
    minutes: number
    sessions: number
  }
}

// Wrapper fn
export const setStats = (stats: Partial<Stats>, batch?: FirebaseFirestoreTypes.WriteBatch) => {
  const ref = getStatsRef()

  if (batch) { batch.set(ref, stats, {merge: true}); return }
  ref.set(stats, { merge: true })
}

// Example call
const batch = firestore().batch()

batch.set(otherRef, {})
setStats({
  allTime: {
    minutes: firestore.FieldValue.increment(1),
    sessions: firestore.FieldValue.increment(1),
  }
}, batch)
batch.commit()

This results in an TS throwing me an error since the generated types only accepts the number primitive. Would it be possible to add FieldValue as an allowed type when providing an int or float type?

bijoutrouvaille commented 4 years ago

@ninjz it's a good question, thanks for bringing it up. The issue here is of balance in complexity between read types and write types (with all the extra operators). In the past I considered generating two sets of TS types for each Fireward type, but it seemed clunky. That intuition turned out to be good, as these days TS is so powerful that it can do that work for you with a simple generic wrapper. Here is an example:

type Stats = {
  allTime: {
    minutes: number
    sessions: number
  }
}

type Firenum = number | ReturnType<typeof firestore.FieldValue.increment>;

type FirestoreIn<T> =
    T extends object
    ? {
        [k in keyof T]: FirestoreIn<T[k]>
    }
    : T extends number
    ? Firenum
    : T;

type StatsIn = FirestoreIn<Stats>;

If you examine StatsIn, you'll see that its type is

type StatsIn = {
    allTime: {
        minutes: Firenum;
        sessions: Firenum;
    };
}

Does that help? Let me know, and maybe in the future we can add the FirestoreIn wrapper to the generated .ts file.

ninjz commented 4 years ago

@bijoutrouvaille thanks for your patience. The wrapper works great. It would be very nice to see this in the generated .ts file.

Though correct me if i'm wrong, but it seems to me that whenever there is a number type it would be possible to perform the increment operation. So why not just by default apply the Firenum type to any number types

bijoutrouvaille commented 4 years ago

So why not just by default apply the Firenum type to any number types

@ninjz In my current understanding, the increment operation is rarely done relative to regular number usage. If we changed number to Firenum, the additional boilerplate would need to be written every time the number is read in an ordinary, non-incremental way. That's what I meant by "balance".

I'm toying with the idea of making a separate type in Fireward, maybe increment, that will compile to a float in Firestore and Firenum in TS. Or should there be two, one for ints, one for floats?

I'll leave this issue open for now to see if anyone else wants to chime in.

ninjz commented 4 years ago

I'm toying with the idea of making a separate type in Fireward, maybe increment, that will compile to a float in Firestore and Firenum in TS. Or should there be two, one for ints, one for floats?

I guess it would make sense to have one for ints and one for floats, seeing as there is already individual support for both of those types.